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.

Sign up for GitHub

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

Jump to bottom

Fix const_err with -(-0.0) #64063

Merged
merged 7 commits into from
Sep 5, 2019
Merged

Fix const_err with -(-0.0) #64063

merged 7 commits into from
Sep 5, 2019

Conversation

JohnTitor
Copy link
Member

Fixes #64059

r? @oli-obk

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 1, 2019
oli-obk
oli-obk requested changes Sep 1, 2019
View reviewed changes
src/librustc_mir/transform/const_prop.rs Outdated Show resolved Hide resolved
src/librustc_mir/transform/const_prop.rs Outdated Show resolved Hide resolved
@oli-obk
Copy link
Contributor

oli-obk commented Sep 1, 2019

I'm surprised this change doesn't cause any reduction in duplicate diagnostics... Not sure what's going on.

If I'm grepping correctly, we don't have a single test running in debug mode and thus producing double the warnings that release mode does.

Can you add a test with // -C overflow-checks=true -O with a bunch of overflow examples and see if there's only one error per overflow?

@oli-obk
Copy link
Contributor

oli-obk commented Sep 1, 2019

Super thanks

@bors r+

@bors
Copy link
Contributor

bors commented Sep 1, 2019

📌 Commit a937d8c has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 1, 2019
Centril added a commit to Centril/rust that referenced this pull request Sep 1, 2019
@Centril
Rollup merge of rust-lang#64063 - JohnTitor:fix-const-err, r=oli-obk
8d704e4 
Fix const_err with `-(-0.0)`

Fixes rust-lang#64059

r? @oli-obk
@Centril Centril mentioned this pull request Sep 1, 2019
Closed
Centril added a commit to Centril/rust that referenced this pull request Sep 3, 2019
@Centril
Rollup merge of rust-lang#64063 - JohnTitor:fix-const-err, r=oli-obk
c1832a6 
Fix const_err with `-(-0.0)`

Fixes rust-lang#64059

r? @oli-obk
@Centril Centril mentioned this pull request Sep 3, 2019
Closed
Centril added a commit to Centril/rust that referenced this pull request Sep 3, 2019
@Centril
Rollup merge of rust-lang#64063 - JohnTitor:fix-const-err, r=oli-obk
cc1c96b 
Fix const_err with `-(-0.0)`

Fixes rust-lang#64059

r? @oli-obk
@Centril Centril mentioned this pull request Sep 3, 2019
Closed
@Centril
Copy link
Contributor

Centril commented Sep 3, 2019

Failed in #64122 (comment), @bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 3, 2019
@JohnTitor
Copy link
Member Author

JohnTitor commented Sep 3, 2019 •
edited
Loading

I'm not sure why the test is failed in x86_64-gnu-nopt (should we modify the test annotation?), any thoughts? @oli-obk

oli-obk
oli-obk reviewed Sep 3, 2019
View reviewed changes
src/test/ui/issues/issue-8460-const2.rs
@@ -0,0 +1,59 @@
// compile-flags: -C overflow-checks=on -O
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe noopt overwrites this -O? That would be weird. Also weird that noopt causes fewer diagnostics... I'm not sure what's going on. You can undo the changes unrelated to -(-0.0) if you want so we can get the PR merged and open an issue about not being able to test noopt properly.

Copy link
Member Author

Choose a reason for hiding this comment

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

The weird thing is that it happened in issue-8460-const.rs not issue-8460-const2.rs. issue-8460-const.rs has no compile flags.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh. That may explain it... not sure what the best thing to do here is. Maybe instead of copying the files we should use the scheme where one test is built in multiple modes? I forget what the appropriate flags are, but incremental has loads of them

Copy link
Member Author

@JohnTitor JohnTitor Sep 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Or could we add the -O flag to issue-8460-const.rs? The other tests we touch have this flag but that doesn't.

Copy link
Contributor

Choose a reason for hiding this comment

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

If that solves it, sure

Copy link
Member Author

Choose a reason for hiding this comment

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

I added the flag to the test, could we run bors try?

Copy link
Contributor

Choose a reason for hiding this comment

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

You can test this locally by setting

rust/config.toml.example

Line 355 in b9de4ef

#optimize-tests = true
to false in your config.toml

Unfortunately bors try won't run the noopt tests, so that's the only way to test

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I didn't know that.
I created config.toml with optimize-tests = false and ran x.py test src/test/ui --stage=1 --bless --config config.toml, the test seems passed.

oli-obk
oli-obk reviewed Sep 3, 2019
View reviewed changes
src/librustc_mir/transform/const_prop.rs
// We check overflow in debug mode already
// so should only check in release mode.
if !oflo_check
&& prim.layout.ty.is_signed()
Copy link
Contributor

Choose a reason for hiding this comment

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

essentially this condition the only needed thing for the fix. The rest is what's causing the CI failure

@oli-obk
Copy link
Contributor

oli-obk commented Sep 4, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Sep 4, 2019

📌 Commit 41deb83 has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 4, 2019
Centril added a commit to Centril/rust that referenced this pull request Sep 5, 2019
@Centril
Rollup merge of rust-lang#64063 - JohnTitor:fix-const-err, r=oli-obk
b683631 
Fix const_err with `-(-0.0)`

Fixes rust-lang#64059

r? @oli-obk
@Centril Centril mentioned this pull request Sep 5, 2019
Closed
Centril added a commit to Centril/rust that referenced this pull request Sep 5, 2019
@Centril
Rollup merge of rust-lang#64063 - JohnTitor:fix-const-err, r=oli-obk
49bc3f5 
Fix const_err with `-(-0.0)`

Fixes rust-lang#64059

r? @oli-obk
@Centril Centril mentioned this pull request Sep 5, 2019
Closed
Centril added a commit to Centril/rust that referenced this pull request Sep 5, 2019
@Centril
Rollup merge of rust-lang#64063 - JohnTitor:fix-const-err, r=oli-obk
2238d19 
Fix const_err with `-(-0.0)`

Fixes rust-lang#64059

r? @oli-obk
@Centril Centril mentioned this pull request Sep 5, 2019
Merged
bors added a commit that referenced this pull request Sep 5, 2019
@bors
Auto merge of #64172 - Centril:rollup-8i8oh54, r=Centril
6187684 
Rollup of 11 pull requests

Successful merges:

 - #62848 (Use unicode-xid crate instead of libcore)
 - #63774 (Fix `window.hashchange is not a function`)
 - #63930 (Account for doc comments coming from proc macros without spans)
 - #64003 (place: Passing `align` = `layout.align.abi`, when also passing `layout`)
 - #64030 (Fix unlock ordering in SGX synchronization primitives)
 - #64041 (use TokenStream rather than &[TokenTree] for built-in macros)
 - #64051 (Add x86_64-linux-kernel target)
 - #64063 (Fix const_err with `-(-0.0)`)
 - #64083 (Point at appropriate arm on type error on if/else/match with one non-! arm)
 - #64100 (Fix const eval bug breaking run-pass tests in Miri)
 - #64157 (Opaque type locations in error message for clarity.)

Failed merges:

r? @ghost
@bors bors merged commit 41deb83 into rust-lang:master Sep 5, 2019
@JohnTitor JohnTitor deleted the fix-const-err branch September 5, 2019 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oli-obk oli-obk oli-obk requested changes

Assignees

@oli-obk oli-obk

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

-(-0.0) raises const_err "attempt to negate with overflow"
5 participants
@JohnTitor @oli-obk @bors @Centril @rust-highfive