Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make tuple constructors real const fns #61209

Merged
merged 3 commits into from Jun 7, 2019

Conversation

Projects
None yet
6 participants
@matthewjasper
Copy link
Contributor

commented May 26, 2019

Mir construction special cases Ctor(...) to be lowered as Ctor { 0: ... }, which means this doesn't come up much in practice, but it seems inconsistent not to allow this.

Tracking issue: #61456

r? @oli-obk

@Centril Centril added this to the 1.37 milestone May 26, 2019

@Centril

This comment was marked as outdated.

Copy link
Member

commented May 26, 2019

Can you write up a longer report for FCP akin to #57175 (comment). Can you also then follow up with a change to https://doc.rust-lang.org/nightly/reference/items/functions.html#const-functions?

Show resolved Hide resolved src/librustc_mir/shim.rs Outdated
Show resolved Hide resolved src/librustc_mir/transform/mod.rs
let gcx = tcx.global_tcx();
let def_id = tcx.hir().local_def_id_from_hir_id(ctor_id);
let param_env = gcx.param_env(def_id);
pub fn build_adt_ctor<'a, 'gcx>(tcx: TyCtxt<'a, 'gcx, 'gcx>, ctor_id: DefId) -> &'gcx Mir<'gcx> {

This comment has been minimized.

Copy link
@oli-obk

oli-obk May 26, 2019

Contributor

This change makes me wonder whether it would be possible to generalize build_adt_ctor to the point where we can scrap mir::AggregateKind::Adt and just call the part of build_adt_ctor that generates the field and discr initialization.

Additionally, if I'm reading this correctly, build_adt_ctor could generate a shim that allows initializing arbitrary ty::Adt. So there's nothing implementation wise that would speak against the lang team figuring out a way to obtain initialization functions for struct variants or just plain structs. cc @Centril

This comment has been minimized.

Copy link
@oli-obk

oli-obk May 26, 2019

Contributor

cc @rust-lang/lang Not suggesting that we add this or anything, but wanted to let you know that we can now generate a function

fn foo(a: A, b: B, c: C) -> Foo {
    Foo { a, b, c }
}

from a struct definition

struct Foo {
    a: A,
    b: B,
    c: C,
}

In case this ever comes up as something that may be useful

This comment has been minimized.

Copy link
@eddyb

eddyb May 28, 2019

Member

@oli-obk FWIW we already have an optimization that expands Rvalue::Aggregate into this instruction sequence so this code is already duplicated, I think?

This comment has been minimized.

Copy link
@oli-obk

oli-obk May 29, 2019

Contributor

iirc that optimization is very expensive to run. I know we want to do little work at MIR building time, but in this case I believe it's equivalent in complexity. We can scrap that optimization if we just do it all at MIR building time.

This comment has been minimized.

Copy link
@eddyb

eddyb May 29, 2019

Member

I meant that we could put the code generating this in that optimization until we get rid of it (rather than relying on running the optimization).

Show resolved Hide resolved src/librustc/ty/constness.rs
Show resolved Hide resolved src/test/run-pass/consts/const-construct-call.rs Outdated

@Centril Centril removed this from the 1.37 milestone May 26, 2019

Show resolved Hide resolved src/librustc_mir/shim.rs Outdated
// (return as Variant).field0 = arg0;
// (return as Variant).field1 = arg1;
//
// discriminant(return) = variant_index;

This comment has been minimized.

Copy link
@eddyb

eddyb May 28, 2019

Member

So I guess this is pre-optimized? As opposed to Rvalue::Aggregate?

@bors

This comment was marked as resolved.

Copy link
Contributor

commented May 29, 2019

☔️ The latest upstream changes (presumably #61305) made this pull request unmergeable. Please resolve the merge conflicts.

Show resolved Hide resolved src/librustc_mir/shim.rs Outdated

@matthewjasper matthewjasper force-pushed the matthewjasper:const-tuple-constructors branch from 03c42ed to 0068ea0 Jun 2, 2019

@bors

This comment was marked as resolved.

Copy link
Contributor

commented Jun 6, 2019

☔️ The latest upstream changes (presumably #57428) made this pull request unmergeable. Please resolve the merge conflicts.

Show resolved Hide resolved src/librustc/ty/constness.rs Outdated
@oli-obk

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

r=me with feature gates cleaned up

@matthewjasper matthewjasper force-pushed the matthewjasper:const-tuple-constructors branch from 0068ea0 to bcf8365 Jun 6, 2019

@matthewjasper

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

@bors r=oli-obk

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

📌 Commit bcf8365 has been approved by oli-obk

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

⌛️ Testing commit bcf8365 with merge c1c60d2...

bors added a commit that referenced this pull request Jun 7, 2019

Auto merge of #61209 - matthewjasper:const-tuple-constructors, r=oli-obk
Make tuple constructors real const fns

Mir construction special cases `Ctor(...)` to be lowered as `Ctor { 0: ... }`, which means this doesn't come up much in practice, but it seems inconsistent not to allow this.

r? @oli-obk
@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: oli-obk
Pushing c1c60d2 to master...

@bors bors added the merged-by-bors label Jun 7, 2019

@bors bors merged commit bcf8365 into rust-lang:master Jun 7, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.