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

do not normalize all non-scalar constants to a ConstValue::ScalarPair #54693

Merged
merged 2 commits into from Oct 2, 2018

Conversation

Projects
None yet
7 participants
@RalfJung
Member

RalfJung commented Sep 30, 2018

We still need ConstValue::ScalarPair for match handling (matching slices and strings), but that will never see anything Undef. For non-fat-ptr ScalarPair, just point to the allocation like larger data structures do.

Fixes #54387

r? @eddyb

@RalfJung

This comment has been minimized.

Show comment
Hide comment
@RalfJung
Member

RalfJung commented Oct 1, 2018

@oli-obk

This comment has been minimized.

Show comment
Hide comment
@oli-obk

oli-obk Oct 1, 2018

Contributor

@bors r+

Contributor

oli-obk commented Oct 1, 2018

@bors r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Oct 1, 2018

Contributor

📌 Commit d62aa3e has been approved by oli-obk

Contributor

bors commented Oct 1, 2018

📌 Commit d62aa3e has been approved by oli-obk

@oli-obk oli-obk assigned oli-obk and unassigned eddyb Oct 1, 2018

@RalfJung

This comment has been minimized.

Show comment
Hide comment
@RalfJung

RalfJung Oct 1, 2018

Member

A less risky variant for beta might be to just backport the changes in const_eval to normalize fewer things, but not get rid of the ScalarMaybeUndef in ConstValue entirely.

Member

RalfJung commented Oct 1, 2018

A less risky variant for beta might be to just backport the changes in const_eval to normalize fewer things, but not get rid of the ScalarMaybeUndef in ConstValue entirely.

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Oct 1, 2018

Contributor

⌛️ Testing commit d62aa3e with merge 7cbcdae...

Contributor

bors commented Oct 1, 2018

⌛️ Testing commit d62aa3e with merge 7cbcdae...

bors added a commit that referenced this pull request Oct 1, 2018

Auto merge of #54693 - RalfJung:ctfe-scalar-pair-undef, r=oli-obk
do not normalize all non-scalar constants to a ConstValue::ScalarPair

We still need `ConstValue::ScalarPair` for match handling (matching slices and strings), but that will never see anything `Undef`. For non-fat-ptr `ScalarPair`, just point to the allocation like larger data structures do.

Fixes #54387

r? @eddyb
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Oct 2, 2018

Contributor

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

Contributor

bors commented Oct 2, 2018

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

@bors bors merged commit d62aa3e into rust-lang:master Oct 2, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Oct 2, 2018

Collaborator

📣 Toolstate changed by #54693!

Tested on commit 7cbcdae.
Direct link to PR: #54693

💔 clippy-driver on windows: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra).
💔 clippy-driver on linux: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra).
💔 miri on windows: test-pass → build-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra).
💔 miri on linux: test-pass → build-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra).
💔 rls on windows: test-pass → build-fail (cc @nrc, @rust-lang/infra).
💔 rls on linux: test-pass → build-fail (cc @nrc, @rust-lang/infra).

Collaborator

rust-highfive commented Oct 2, 2018

📣 Toolstate changed by #54693!

Tested on commit 7cbcdae.
Direct link to PR: #54693

💔 clippy-driver on windows: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra).
💔 clippy-driver on linux: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra).
💔 miri on windows: test-pass → build-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra).
💔 miri on linux: test-pass → build-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra).
💔 rls on windows: test-pass → build-fail (cc @nrc, @rust-lang/infra).
💔 rls on linux: test-pass → build-fail (cc @nrc, @rust-lang/infra).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Oct 2, 2018

📣 Toolstate changed by rust-lang/rust#54693!
Tested on commit rust-lang/rust@7cbcdae.
Direct link to PR: <rust-lang/rust#54693>

💔 clippy-driver on windows: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra).
💔 clippy-driver on linux: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra).
💔 miri on windows: test-pass → build-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra).
💔 miri on linux: test-pass → build-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra).
💔 rls on windows: test-pass → build-fail (cc @nrc, @rust-lang/infra).
💔 rls on linux: test-pass → build-fail (cc @nrc, @rust-lang/infra).
@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Oct 2, 2018

Member

So the argument here is that anything that doesn't need an allocation should be fully initialized?
(I expected ScalarMaybeUndef to be used more, but I see why you'd want the opposite)

Member

eddyb commented Oct 2, 2018

So the argument here is that anything that doesn't need an allocation should be fully initialized?
(I expected ScalarMaybeUndef to be used more, but I see why you'd want the opposite)

@RalfJung RalfJung deleted the RalfJung:ctfe-scalar-pair-undef branch Oct 2, 2018

@RalfJung

This comment has been minimized.

Show comment
Hide comment
@RalfJung

RalfJung Oct 2, 2018

Member

So the argument here is that anything that doesn't need an allocation should be fully initialized?

Yes. We have those variants only because pattern matching and some code around array lengths need them. No reason they should have to handle Undef.

Everything gets an allocation anyway from the way const_eval works, it's just for some types we decide to get stuff out of the allocation earlier. It's a bit messy, and I'd prefer if these were different types and we had one query that always returns an allocation and another than always returns an immediate (and is only used in a few places).

Member

RalfJung commented Oct 2, 2018

So the argument here is that anything that doesn't need an allocation should be fully initialized?

Yes. We have those variants only because pattern matching and some code around array lengths need them. No reason they should have to handle Undef.

Everything gets an allocation anyway from the way const_eval works, it's just for some types we decide to get stuff out of the allocation earlier. It's a bit messy, and I'd prefer if these were different types and we had one query that always returns an allocation and another than always returns an immediate (and is only used in a few places).

@RalfJung

This comment has been minimized.

Show comment
Hide comment
@RalfJung

RalfJung Oct 2, 2018

Member

Also see #54738

Member

RalfJung commented Oct 2, 2018

Also see #54738

@RalfJung

This comment has been minimized.

Show comment
Hide comment
@RalfJung

RalfJung Oct 2, 2018

Member

I made a beta backport at #54759

Member

RalfJung commented Oct 2, 2018

I made a beta backport at #54759

@RalfJung

This comment has been minimized.

Show comment
Hide comment
@RalfJung

RalfJung Oct 2, 2018

Member

Oh dang. Now I see @oli-obk already did that. oops^^

Member

RalfJung commented Oct 2, 2018

Oh dang. Now I see @oli-obk already did that. oops^^

@pnkfelix pnkfelix removed the beta-nominated label Oct 4, 2018

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Oct 4, 2018

Member

T-compiler accepted #54759 for the backport.

Member

pnkfelix commented Oct 4, 2018

T-compiler accepted #54759 for the backport.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment