add debug_assert_ne + assert_ne #35074

Merged
merged 1 commit into from Sep 21, 2016

Conversation

Projects
None yet
7 participants
@ashleygwilliams
Member

ashleygwilliams commented Jul 27, 2016

work in progress, please do not merge

fixes #35073

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Jul 27, 2016

Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (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.

Collaborator

rust-highfive commented Jul 27, 2016

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (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.

@ashleygwilliams ashleygwilliams changed the title from [WIP] add debug_assert_ne to [WIP] add debug_assert_ne + assert_ne Jul 27, 2016

src/libcore/macros.rs
+/// assert_ne!(a, b);
+/// ```
+#[macro_export]
+///#[stable(feature = "????", since = "????")]

This comment has been minimized.

@ashleygwilliams

ashleygwilliams Jul 27, 2016

Member

not sure what to put here

@ashleygwilliams

ashleygwilliams Jul 27, 2016

Member

not sure what to put here

This comment has been minimized.

@steveklabnik

steveklabnik Jul 27, 2016

Member

macros are a bit odd, since they become insta-stable. Right now, since should be 1.12.0. Unsure about feature, since it won't have one. @alexcrichton ?

@steveklabnik

steveklabnik Jul 27, 2016

Member

macros are a bit odd, since they become insta-stable. Right now, since should be 1.12.0. Unsure about feature, since it won't have one. @alexcrichton ?

This comment has been minimized.

@aturon

aturon Jul 27, 2016

Member

I think the feature effectively doesn't matter -- assert_ne would be fine.

@aturon

aturon Jul 27, 2016

Member

I think the feature effectively doesn't matter -- assert_ne would be fine.

This comment has been minimized.

@alexcrichton

alexcrichton Jul 28, 2016

Member

(what @aturon said)

src/libcore/macros.rs
+/// debug_assert_ne!(a, b);
+/// ```
+#[macro_export]
+/// #[stable(feature = "????", since = "?????")]

This comment has been minimized.

@ashleygwilliams

ashleygwilliams Jul 27, 2016

Member

same as above, not sure what to put here

@ashleygwilliams

ashleygwilliams Jul 27, 2016

Member

same as above, not sure what to put here

+ match (&$left, &$right) {
+ (left_val, right_val) => {
+ if *left_val == *right_val {
+ panic!("assertion failed: `(left != right)` \

This comment has been minimized.

@alexcrichton

alexcrichton Jul 28, 2016

Member

s/!=/==/

This comment has been minimized.

@alexcrichton

alexcrichton Jul 28, 2016

Member

(same below)

@alexcrichton

alexcrichton Jul 28, 2016

Member

(same below)

This comment has been minimized.

@ashleygwilliams

ashleygwilliams Aug 17, 2016

Member

i think this is correct based on the pattern established by assert_eq (https://github.com/rust-lang/rust/blob/master/src/libcore/macros.rs#L103). though i'll agree it is a bit ambiguous sans the context of assert_eq's impl.

@ashleygwilliams

ashleygwilliams Aug 17, 2016

Member

i think this is correct based on the pattern established by assert_eq (https://github.com/rust-lang/rust/blob/master/src/libcore/macros.rs#L103). though i'll agree it is a bit ambiguous sans the context of assert_eq's impl.

This comment has been minimized.

@alexcrichton

alexcrichton Aug 18, 2016

Member

Here though the assertion that's failing is that (left == right), not (left != right), right?

Otherwise, if I write assert_ne!(1, 1) it'll print:

assertion failed: (1 != 1)
@alexcrichton

alexcrichton Aug 18, 2016

Member

Here though the assertion that's failing is that (left == right), not (left != right), right?

Otherwise, if I write assert_ne!(1, 1) it'll print:

assertion failed: (1 != 1)

This comment has been minimized.

@ashleygwilliams

ashleygwilliams Aug 31, 2016

Member

i see you point, though i don't think you are right. i think it should be read something like: assertion failed: (<insert the assertion that tried and failed here>).

for example:
with assert_eq it would print: assertion failed: (6 == 7)
https://play.rust-lang.org/?gist=d2cee1db97caf48c7ad7efc4c6cdc88f&version=stable&backtrace=0

as it is currently written, the assert_ne message is in keeping with the assert_eq behavior.

@ashleygwilliams

ashleygwilliams Aug 31, 2016

Member

i see you point, though i don't think you are right. i think it should be read something like: assertion failed: (<insert the assertion that tried and failed here>).

for example:
with assert_eq it would print: assertion failed: (6 == 7)
https://play.rust-lang.org/?gist=d2cee1db97caf48c7ad7efc4c6cdc88f&version=stable&backtrace=0

as it is currently written, the assert_ne message is in keeping with the assert_eq behavior.

This comment has been minimized.

@alexcrichton

alexcrichton Aug 31, 2016

Member

Wow I'm just thinking totally backwards, carry on!

@alexcrichton

alexcrichton Aug 31, 2016

Member

Wow I'm just thinking totally backwards, carry on!

src/libcore/macros.rs
+/// ```
+#[macro_export]
+///#[stable(feature = "????", since = "????")]
+macro_rules! assert_eq {

This comment has been minimized.

@cgswords

cgswords Jul 29, 2016

Contributor

pretty sure this should be assert_ne

@cgswords

cgswords Jul 29, 2016

Contributor

pretty sure this should be assert_ne

@ashleygwilliams

This comment has been minimized.

Show comment
Hide comment
@ashleygwilliams

ashleygwilliams Aug 17, 2016

Member

hi! does this need tests? i'm struggling to find where assert_eq is tested. it is not in the same file, and a search of this repo for assert_eq and test leads to a lot of results 😬

would love some direction on where/if to write tests. otherwise i think this should be good to go!

Member

ashleygwilliams commented Aug 17, 2016

hi! does this need tests? i'm struggling to find where assert_eq is tested. it is not in the same file, and a search of this repo for assert_eq and test leads to a lot of results 😬

would love some direction on where/if to write tests. otherwise i think this should be good to go!

@ashleygwilliams

This comment has been minimized.

Show comment
Hide comment
@ashleygwilliams

ashleygwilliams Aug 17, 2016

Member

ok! tests added. thanks for the point in the right direction @steveklabnik 🐬

Member

ashleygwilliams commented Aug 17, 2016

ok! tests added. thanks for the point in the right direction @steveklabnik 🐬

@ashleygwilliams ashleygwilliams changed the title from [WIP] add debug_assert_ne + assert_ne to add debug_assert_ne + assert_ne Aug 17, 2016

@@ -0,0 +1,12 @@
+#[derive(PartialEq, Debug)]

This comment has been minimized.

@alexcrichton

alexcrichton Aug 18, 2016

Member

I think this may need our standard license header to pass make tidy

@alexcrichton

alexcrichton Aug 18, 2016

Member

I think this may need our standard license header to pass make tidy

This comment has been minimized.

@ashleygwilliams

ashleygwilliams Aug 31, 2016

Member

got it, @steveklabnik said it didn't, but i'll add it

@ashleygwilliams

ashleygwilliams Aug 31, 2016

Member

got it, @steveklabnik said it didn't, but i'll add it

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
Member

alexcrichton commented Aug 31, 2016

@ashleygwilliams

This comment has been minimized.

Show comment
Hide comment
@ashleygwilliams

ashleygwilliams Aug 31, 2016

Member

lemme know if you'd like me to squash ;)

Member

ashleygwilliams commented Aug 31, 2016

lemme know if you'd like me to squash ;)

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Aug 31, 2016

Contributor

⌛️ Testing commit 3f404f4 with merge aa5ce78...

Contributor

bors commented Aug 31, 2016

⌛️ Testing commit 3f404f4 with merge aa5ce78...

bors added a commit that referenced this pull request Aug 31, 2016

Auto merge of #35074 - ashleygwilliams:assert_ne, r=alexcrichton
add debug_assert_ne + assert_ne

~~ work in progress, please do not merge ~~

fixes #35073
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Aug 31, 2016

Contributor

💔 Test failed - auto-linux-64-nopt-t

Contributor

bors commented Aug 31, 2016

💔 Test failed - auto-linux-64-nopt-t

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Aug 31, 2016

Member

@ashleygwilliams sure! Looks like the stability attributes may also need to be uncommented?

Member

alexcrichton commented Aug 31, 2016

@ashleygwilliams sure! Looks like the stability attributes may also need to be uncommented?

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
Member

alexcrichton commented Sep 1, 2016

@bors: r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Sep 1, 2016

Contributor

📌 Commit ec8a8ef has been approved by alexcrichton

Contributor

bors commented Sep 1, 2016

📌 Commit ec8a8ef has been approved by alexcrichton

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Sep 1, 2016

Contributor

⌛️ Testing commit ec8a8ef with merge 8e68479...

Contributor

bors commented Sep 1, 2016

⌛️ Testing commit ec8a8ef with merge 8e68479...

bors added a commit that referenced this pull request Sep 1, 2016

Auto merge of #35074 - ashleygwilliams:assert_ne, r=alexcrichton
add debug_assert_ne + assert_ne

~~ work in progress, please do not merge ~~

fixes #35073
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Sep 1, 2016

Contributor

💔 Test failed - auto-mac-64-opt-rustbuild

Contributor

bors commented Sep 1, 2016

💔 Test failed - auto-mac-64-opt-rustbuild

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Sep 1, 2016

Member

Hmm, I don't see why this failed.

Member

steveklabnik commented Sep 1, 2016

Hmm, I don't see why this failed.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Sep 1, 2016

Member

Hm I guess maybe that could be a bug in rustbuild where it's using the wrong standard library, but I'd be surprised if that just came up!

@ashleygwilliams does make check-stage2-rpass pass locally for you?

Member

alexcrichton commented Sep 1, 2016

Hm I guess maybe that could be a bug in rustbuild where it's using the wrong standard library, but I'd be surprised if that just came up!

@ashleygwilliams does make check-stage2-rpass pass locally for you?

@ashleygwilliams

This comment has been minimized.

Show comment
Hide comment
@ashleygwilliams

ashleygwilliams Sep 1, 2016

Member

@alexcrichton make: *** No rule to make targetcheck-stage2-rpass'. Stop.`

both @steveklabnik and i aren't sure what this error means

Member

ashleygwilliams commented Sep 1, 2016

@alexcrichton make: *** No rule to make targetcheck-stage2-rpass'. Stop.`

both @steveklabnik and i aren't sure what this error means

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Sep 1, 2016

Member

@ashleygwilliams of did you pass --enable-rustbuild to configure? In that case the command would be:

python src/bootstrap/bootstrap.py --step check-rpass 
Member

alexcrichton commented Sep 1, 2016

@ashleygwilliams of did you pass --enable-rustbuild to configure? In that case the command would be:

python src/bootstrap/bootstrap.py --step check-rpass 
@ashleygwilliams

This comment has been minimized.

Show comment
Hide comment
@ashleygwilliams

ashleygwilliams Sep 1, 2016

Member

am waiting on it now @alexcrichton

Member

ashleygwilliams commented Sep 1, 2016

am waiting on it now @alexcrichton

@ashleygwilliams

This comment has been minimized.

Show comment
Hide comment
@ashleygwilliams

ashleygwilliams Sep 1, 2016

Member

fails the same on my local machine as it fails in travis.

Member

ashleygwilliams commented Sep 1, 2016

fails the same on my local machine as it fails in travis.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Sep 2, 2016

Member

Oh right these macros are defined in libcore but need to be reexported from the standard library

Member

alexcrichton commented Sep 2, 2016

Oh right these macros are defined in libcore but need to be reexported from the standard library

@ashleygwilliams

This comment has been minimized.

Show comment
Hide comment
@ashleygwilliams

ashleygwilliams Sep 2, 2016

Member

lol omg I KNEW IT. i had been bugging @steveklabnik about needing to "register" the macros somewhere. commit + squash coming up

Member

ashleygwilliams commented Sep 2, 2016

lol omg I KNEW IT. i had been bugging @steveklabnik about needing to "register" the macros somewhere. commit + squash coming up

src/libstd/lib.rs
@@ -302,7 +302,7 @@ use prelude::v1::*;
// We want to reexport a few macros from core but libcore has already been
// imported by the compiler (via our #[no_std] attribute) In this case we just
// add a new crate name so we can attach the reexports to it.
-#[macro_reexport(assert, assert_eq, debug_assert, debug_assert_eq,
+#[macro_reexport(assert, assert_eq, assert_ne, debug_assert, debug_assert_eq,

This comment has been minimized.

@alexcrichton

alexcrichton Sep 2, 2016

Member

debug_assert_ne as well?

@alexcrichton

alexcrichton Sep 2, 2016

Member

debug_assert_ne as well?

@ashleygwilliams

This comment has been minimized.

Show comment
Hide comment
@ashleygwilliams

ashleygwilliams Sep 2, 2016

Member
test result: ok. 2445 passed; 0 failed; 60 ignored; 0 measured
The command "docker run -v `pwd`:/build rust sh -c " ./configure --llvm-root=/usr/lib/llvm-3.7 && make tidy && make check-notidy -j4 "" exited with 2.
Done. Your build exited with 1.

i am unclear what this means.

Member

ashleygwilliams commented Sep 2, 2016

test result: ok. 2445 passed; 0 failed; 60 ignored; 0 measured
The command "docker run -v `pwd`:/build rust sh -c " ./configure --llvm-root=/usr/lib/llvm-3.7 && make tidy && make check-notidy -j4 "" exited with 2.
Done. Your build exited with 1.

i am unclear what this means.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Sep 2, 2016

Member

@bors: r+

Ah yeah that's fine, travis is broken right now unfortunately :(

Member

alexcrichton commented Sep 2, 2016

@bors: r+

Ah yeah that's fine, travis is broken right now unfortunately :(

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Sep 2, 2016

Contributor

📌 Commit 01d085b has been approved by alexcrichton

Contributor

bors commented Sep 2, 2016

📌 Commit 01d085b has been approved by alexcrichton

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Sep 2, 2016

Contributor

⌛️ Testing commit 01d085b with merge 848e652...

Contributor

bors commented Sep 2, 2016

⌛️ Testing commit 01d085b with merge 848e652...

bors added a commit that referenced this pull request Sep 2, 2016

Auto merge of #35074 - ashleygwilliams:assert_ne, r=alexcrichton
add debug_assert_ne + assert_ne

~~ work in progress, please do not merge ~~

fixes #35073
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Sep 3, 2016

Contributor

💔 Test failed - auto-linux-32-nopt-t

Contributor

bors commented Sep 3, 2016

💔 Test failed - auto-linux-32-nopt-t

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
Member

steveklabnik commented Sep 3, 2016

@bors: retry

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Sep 3, 2016

Contributor

🔒 Merge conflict

Contributor

bors commented Sep 3, 2016

🔒 Merge conflict

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Sep 21, 2016

Member

/cc @ashleygwilliams , let's get that merge conflict fixed

Member

steveklabnik commented Sep 21, 2016

/cc @ashleygwilliams , let's get that merge conflict fixed

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
Member

steveklabnik commented Sep 21, 2016

@bors: r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Sep 21, 2016

Contributor

📌 Commit 3d8d557 has been approved by steveklabnik

Contributor

bors commented Sep 21, 2016

📌 Commit 3d8d557 has been approved by steveklabnik

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Sep 21, 2016

Member

Homu still seems to think this isn't mergeable @alexcrichton :/

Member

steveklabnik commented Sep 21, 2016

Homu still seems to think this isn't mergeable @alexcrichton :/

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Sep 21, 2016

Member

Actually, it appears it was just a bad cache, it's working now 👍

Member

steveklabnik commented Sep 21, 2016

Actually, it appears it was just a bad cache, it's working now 👍

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Sep 21, 2016

Contributor

⌛️ Testing commit 3d8d557 with merge 53f9730...

Contributor

bors commented Sep 21, 2016

⌛️ Testing commit 3d8d557 with merge 53f9730...

bors added a commit that referenced this pull request Sep 21, 2016

Auto merge of #35074 - ashleygwilliams:assert_ne, r=steveklabnik
add debug_assert_ne + assert_ne

~~ work in progress, please do not merge ~~

fixes #35073

@bors bors merged commit 3d8d557 into rust-lang:master Sep 21, 2016

1 check passed

homu Test successful
Details

kumbayo added a commit to kumbayo/intellij-rust that referenced this pull request Oct 7, 2016

(GRAM): Recognize assert!, assert_eq! and assert_ne! macros
This improves navigation and code highlighting inside the following macros:
assert!(a == b);
debug_assert!(a == b);
assert_eq!(a, b);
debug_assert_eq!(a, b);
assert_ne!(a, b);
debug_assert_ne!(a, b);

For assert! and debug_assert! format arguments are supported:
assert!(a == b, "Some text");
assert!(a == b, "Text {} {} syntax", "with", "format");

Different parenthesis are supported:
assert!(a == b);
assert![a == b];
assert!{a == b};

Fixes #636

assert_ne! is stable since Rust 1.12.
rust-lang/rust#35074

kumbayo added a commit to kumbayo/intellij-rust that referenced this pull request Oct 7, 2016

(GRAM): Recognize assert!, assert_eq! and assert_ne! macros
This improves navigation and code highlighting inside the following macros:
assert!(a == b);
debug_assert!(a == b);
assert_eq!(a, b);
debug_assert_eq!(a, b);
assert_ne!(a, b);
debug_assert_ne!(a, b);

Format arguments are supported:
assert!(a == b, "Text {} {} syntax", "with", "format");
assert!(a == b, "Some text");
assert_eq!(a, b, "Some text");
assert_ne!(a, b, "Some text");

Different parenthesis are supported:
assert!(a == b);
assert![a == b];
assert!{a == b};

Fixes #636

assert_ne! is stable since Rust 1.12.
rust-lang/rust#35074
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment