Skip to content

Add lint check for negating uint literals and variables. #13579

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
Merged
merged 1 commit into from
May 3, 2014
Merged

Add lint check for negating uint literals and variables. #13579

merged 1 commit into from
May 3, 2014

Conversation

hirschenberger
Copy link
Contributor

See #11273 and #13318

@hirschenberger
Copy link
Contributor Author

Travis failed because he doesn't know the new lint check in stage1.

@hirschenberger
Copy link
Contributor Author

@brson Can you help me? I don't know how to satisfy @bors.

@alexcrichton
Copy link
Member

If you take a look at the logs that are preceded with "failure:" you can see the logs of the otuput that buildbot collected during the run.

It looks like there are some warnings in the standard library which should be addressed.

@hirschenberger
Copy link
Contributor Author

I think the Problem is, that the stage0 compiler doesn't know the new lint rule:

rust/src/librustc/middle/trans/adt.rs:46:10: 46:25 warning: unknown `allow` attribute: `unsigned_negate`, #[warn(unrecognized_lint)] on by default

and bails out later. But it may also be that it doesn't recognize my #![allow(unsigned_negate)] because of the macro magic going on in uint_macros.rs.

If I compile with make RUSTFLAGS="-A unsigned-negate" it works.

@alexcrichton
Copy link
Member

The warnings are ok because they all go away after stage0, you'll want to look near the ends of the logs rather than the top:

/home/rustbuild/src/rust-buildbot/slave/auto-linux-64-x-android-t/build/src/libstd/num/uint_macros.rs:99:27: 99:33 error: negation of unsigned int may be unintentional, #[deny(unsigned_negate)] on by default
/home/rustbuild/src/rust-buildbot/slave/auto-linux-64-x-android-t/build/src/libstd/num/uint_macros.rs:99     fn neg(&self) -> $T { -*self }
                                                                                                                                   ^~~~~~
/home/rustbuild/src/rust-buildbot/slave/auto-linux-64-x-android-t/build/src/libstd/num/uint_macros.rs:15:1: 396:3 note: in expansion of uint_module!
/home/rustbuild/src/rust-buildbot/slave/auto-linux-64-x-android-t/build/src/libstd/num/uint.rs:27:1: 27:37 note: expansion site
/home/rustbuild/src/rust-buildbot/slave/auto-linux-64-x-android-t/build/src/libstd/num/uint_macros.rs:99:27: 99:33 error: negation of unsigned int may be unintentional, #[deny(unsigned_negate)] on by default
/home/rustbuild/src/rust-buildbot/slave/auto-linux-64-x-android-t/build/src/libstd/num/uint_macros.rs:99     fn neg(&self) -> $T { -*self }
                                                                                                                                   ^~~~~~
/home/rustbuild/src/rust-buildbot/slave/auto-linux-64-x-android-t/build/src/libstd/num/uint_macros.rs:15:1: 396:3 note: in expansion of uint_module!
/home/rustbuild/src/rust-buildbot/slave/auto-linux-64-x-android-t/build/src/libstd/num/u8.rs:27:1: 27:24 note: expansion site
/home/rustbuild/src/rust-buildbot/slave/auto-linux-64-x-android-t/build/src/libstd/num/uint_macros.rs:99:27: 99:33 error: negation of unsigned int may be unintentional, #[deny(unsigned_negate)] on by default
/home/rustbuild/src/rust-buildbot/slave/auto-linux-64-x-android-t/build/src/libstd/num/uint_macros.rs:99     fn neg(&self) -> $T { -*self }
                                                                                                                                   ^~~~~~
/home/rustbuild/src/rust-buildbot/slave/auto-linux-64-x-android-t/build/src/libstd/num/uint_macros.rs:15:1: 396:3 note: in expansion of uint_module!
/home/rustbuild/src/rust-buildbot/slave/auto-linux-64-x-android-t/build/src/libstd/num/u16.rs:27:1: 27:27 note: expansion site
/home/rustbuild/src/rust-buildbot/slave/auto-linux-64-x-android-t/build/src/libstd/num/uint_macros.rs:99:27: 99:33 error: negation of unsigned int may be unintentional, #[deny(unsigned_negate)] on by default
/home/rustbuild/src/rust-buildbot/slave/auto-linux-64-x-android-t/build/src/libstd/num/uint_macros.rs:99     fn neg(&self) -> $T { -*self }
                                                                                                                                   ^~~~~~
/home/rustbuild/src/rust-buildbot/slave/auto-linux-64-x-android-t/build/src/libstd/num/uint_macros.rs:15:1: 396:3 note: in expansion of uint_module!
/home/rustbuild/src/rust-buildbot/slave/auto-linux-64-x-android-t/build/src/libstd/num/u32.rs:27:1: 27:27 note: expansion site
/home/rustbuild/src/rust-buildbot/slave/auto-linux-64-x-android-t/build/src/libstd/num/uint_macros.rs:99:27: 99:33 error: negation of unsigned int may be unintentional, #[deny(unsigned_negate)] on by default
/home/rustbuild/src/rust-buildbot/slave/auto-linux-64-x-android-t/build/src/libstd/num/uint_macros.rs:99     fn neg(&self) -> $T { -*self }
                                                                                                                                   ^~~~~~
/home/rustbuild/src/rust-buildbot/slave/auto-linux-64-x-android-t/build/src/libstd/num/uint_macros.rs:15:1: 396:3 note: in expansion of uint_module!
/home/rustbuild/src/rust-buildbot/slave/auto-linux-64-x-android-t/build/src/libstd/num/u64.rs:29:1: 29:27 note: expansion site
error: aborting due to 5 previous errors
make: *** [x86_64-unknown-linux-gnu/stage1/lib/rustlib/x86_64-unknown-linux-gnu/lib/stamp.std] Error 101

@hirschenberger
Copy link
Contributor Author

Ok, works now but how about looking at the places where I had to explicitly allow unsigned negation for possible bugs?
Some places are mentioned here #11273 (comment)

alexcrichton
alexcrichton reviewed May 2, 2014
View reviewed changes
src/librustc/middle/lint.rs
_ => ()
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Sorry it took awhile to get to this, it fell under my radar!

Our code conventions dictate 4-space tabs, could you re-tab these? Additionally, did the first case catch use cases that the second case didn't catch? I would have expected ty::get(t).sty to be ty_uint for uint literals.

@hirschenberger
Copy link
Contributor Author

Ok, I retabbed the code.

Yes the second case would also work for uint literals, but I wanted to differentiate between these two to give a more precise warning message.

@hirschenberger
Copy link
Contributor Author

Sorry, I forgot to adapt the testcase to the new warning text

@alexcrichton
Copy link
Member

It looks like there was also a failure on windows: http://buildbot.rust-lang.org/builders/auto-win-32-nopt-t/builds/4747/steps/test/logs/stdio

@alexcrichton
Copy link
Member

Oops, this is the right link: http://buildbot.rust-lang.org/builders/auto-win-32-nopt-c/builds/4733/steps/compile/logs/stdio

@hirschenberger
Copy link
Contributor Author

Ok, should I change the signedness of the expression or add an allow attribute?

@alexcrichton
Copy link
Member

It's always best to fix it at the source if possible, but it's also ok to fix it up after-the fact.

@hirschenberger
Copy link
Contributor Author

I can't test it on windows, will let due = -(msecs as int64 * 10000) as libc::LARGE_INTEGER; do it?

@alexcrichton
Copy link
Member

That will likely do the trick, yes.

@hirschenberger
Copy link
Contributor Author

Ok, I changed it

bors added a commit that referenced this pull request May 2, 2014
@bors
auto merge of #13579 : hirschenberger/rust/lint_unsigned_negate, r=al…
e0d261e 
…excrichton

See #11273 and #13318
@bors bors closed this May 3, 2014
@bors bors merged commit 6c26cbb into rust-lang:master May 3, 2014
@hirschenberger hirschenberger deleted the lint_unsigned_negate branch May 3, 2014 01:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hirschenberger @alexcrichton @bors