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

Ignore unsopported constant expr error #46570

Merged
merged 2 commits into from
Dec 13, 2017
Merged

Conversation

AgustinCB
Copy link
Contributor

@AgustinCB AgustinCB commented Dec 7, 2017

Fixes #46553

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @pnkfelix (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can write fixes instead of addresses, then github will automatically close the issue when the PR is merged

// except according to those terms.

#![feature(const_fn)]
#![warn(const_err)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be deny, because otherwise the test won't fail if the warning is reintroduced

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 8, 2017
@mooman219
Copy link

mooman219 commented Dec 8, 2017

From what I understand (quoting what was mentioned in IRC), 'the real issue is that rustc has two const evaluators. One for stuff like array lengths, and one for everything else. You are seeing a warning from the first one, but the second one processes this just fine.'

Is suppressing the error like this properly handling the issue?

Edit: Just realized that's quoting @oli-obk, who reviewed this PR. Are those comments still relevant?

@carols10cents
Copy link
Member

review ping for you @pnkfelix !

@arielb1
Copy link
Contributor

arielb1 commented Dec 12, 2017

r? @eddyb

@eddyb
Copy link
Member

eddyb commented Dec 12, 2017

LGTM. @bors delegate=oli-obk

@bors
Copy link
Contributor

bors commented Dec 12, 2017

✌️ @oli-obk can now approve this pull request

@oli-obk
Copy link
Contributor

oli-obk commented Dec 12, 2017

Yea suppressing this seems fine to me, since the lint is meant to report const eval failures early. Implementation deficiencies in the hir const evaluator could be fixed, but it would be wasted work with miri being so close.

@bors r+

@bors
Copy link
Contributor

bors commented Dec 12, 2017

📌 Commit cbd25ed has been approved by oli-obk

@bors
Copy link
Contributor

bors commented Dec 12, 2017

⌛ Testing commit cbd25ed with merge 442b7bd...

bors added a commit that referenced this pull request Dec 12, 2017
Ignore `unsopported constant expr` error

Fixes #46553
@bors
Copy link
Contributor

bors commented Dec 13, 2017

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

@bors bors merged commit cbd25ed into rust-lang:master Dec 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants