Add unary and binary tests for incr-comp #37610

zimurgh · 2016-11-06T01:53:17Z

This is my draft of tests for unary and binary expressions as desired by #37520 for use in the test suite for hashes in incremental compilation. Feedback would be wonderful, if there's any changes I need to make I would appreciate the code review.

?r @michaelwoerister

rust-highfive · 2016-11-06T01:53:31Z

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

alexcrichton · 2016-11-06T02:00:26Z

r? @michaelwoerister

michaelwoerister

Thanks for the PR, @OldManMike!
Except for the "only change"-cases and some nits this looks very good already. If you fix the things I commented on, this is good to go.

(...plus adapt the comment at the top that is still talking about structs)

michaelwoerister · 2016-11-07T14:08:32Z

src/test/incremental/hashes/unary_and_binary_exprs.rs

+#[rustc_metadata_dirty(cfg="cfail2")]
+#[rustc_metadata_clean(cfg="cfail3")]
+pub fn var_negation(y: i32) -> i32 {
+    -y


In this case we have to make sure that y already exists in the cfail1 version (and x still in the later version). Otherwise the hashing algorithm might miss the operand but we wouldn't know because the hash was changed anyway by renaming the function argument. Changing the operand should really be the only change.

michaelwoerister · 2016-11-07T14:08:45Z

src/test/incremental/hashes/unary_and_binary_exprs.rs

+#[rustc_metadata_dirty(cfg="cfail2")]
+#[rustc_metadata_clean(cfg="cfail3")]
+pub fn var_bitwise_not(y: i32) -> i32 {
+    !y


Same as above

michaelwoerister · 2016-11-07T14:09:12Z

src/test/incremental/hashes/unary_and_binary_exprs.rs

+#[rustc_metadata_dirty(cfg="cfail2")]
+#[rustc_metadata_clean(cfg="cfail3")]
+pub fn var_deref(y: &i32) -> i32 {
+    *y


Same as above

michaelwoerister · 2016-11-07T14:09:37Z

src/test/incremental/hashes/unary_and_binary_exprs.rs

+#[rustc_metadata_dirty(cfg="cfail2")]
+#[rustc_metadata_clean(cfg="cfail3")]
+pub fn first_var_add(b: i32) -> i32 {
+    b + 3


Same as above

michaelwoerister · 2016-11-07T14:09:49Z

src/test/incremental/hashes/unary_and_binary_exprs.rs

+#[rustc_metadata_dirty(cfg="cfail2")]
+#[rustc_metadata_clean(cfg="cfail3")]
+pub fn second_var_add(b: i32) -> i32 {
+    1 + b


Same as above

michaelwoerister · 2016-11-07T14:13:15Z

src/test/incremental/hashes/unary_and_binary_exprs.rs

+#[rustc_metadata_dirty(cfg="cfail2")]
+#[rustc_metadata_clean(cfg="cfail3")]
+pub fn type_cast(a: u8) -> u64 {
+    let b = a as u32;


That's a nice way of making this work!

michaelwoerister · 2016-11-07T14:20:22Z

src/test/incremental/hashes/unary_and_binary_exprs.rs

+// Change l-value in assignment ------------------------------------------------
+#[cfg(cfail1)]
+pub fn lvalue() -> i32 {
+    let x = 10;


A let statement is different from an assignment, as far as the compiler is concerned. It's an assignment when writing into an existing variable. So this should be something like:

pub fn lvalue() -> i32 { let mut x = 1; let mut y = 2; x = 10; y }

Note, that I also already added y so that changing the left-hand side of the assignment is really the only change.

michaelwoerister · 2016-11-07T14:20:39Z

src/test/incremental/hashes/unary_and_binary_exprs.rs

+// Change r-value in assignment ------------------------------------------------
+#[cfg(cfail1)]
+pub fn rvalue() -> i32 {
+    let x = 10;


Same as above

michaelwoerister · 2016-11-07T14:26:23Z

src/test/incremental/hashes/unary_and_binary_exprs.rs

+
+// Change index into slice -----------------------------------------------------
+#[cfg(cfail1)]
+pub fn index_to_slice() -> i32 {


This is a bit different than I imagined (what you implemented is something that I want to cover in a separate suite of tests for let statements). Here I'd like the indexing expression in a slice to be tested:

pub fn slice_index(s: &[u8], i: usize, j: usize) -> u8 { s[i] } // becomes pub fn slice_index(s: &[u8], i: usize, j: usize) -> u8 { s[j] }

Said code review and recommendations can be found here: rust-lang#37610

michaelwoerister

I added some more comments.

michaelwoerister · 2016-11-07T19:01:15Z

src/test/incremental/hashes/unary_and_binary_exprs.rs

@@ -148,14 +155,15 @@ pub fn first_var_add(a: i32) -> i32 {
 #[rustc_clean(label="Hir", cfg="cfails3")]
 #[rustc_metadata_dirty(cfg="cfail2")]
 #[rustc_metadata_clean(cfg="cfail3")]
-pub fn first_var_add(b: i32) -> i32 {
+pub fn first_var_add(a: i32, b: i32) -> i32 {
    b + 3


This should be b + 2, otherwise we change both operands.

michaelwoerister · 2016-11-07T19:04:02Z

src/test/incremental/hashes/unary_and_binary_exprs.rs

-    let y = 10;
+    let mut x = 10;
+    let mut y = 11;
+    x = 9;
    y


We want to change the left-hand side of the assignment here, not the return value of the function.

michaelwoerister · 2016-11-07T19:05:32Z

src/test/incremental/hashes/unary_and_binary_exprs.rs

@@ -463,7 +480,9 @@ pub fn lvalue() -> i32 {
 // Change r-value in assignment ------------------------------------------------
 #[cfg(cfail1)]
 pub fn rvalue() -> i32 {
-    let x = 10;
+    let mut x = 10;
+    let mut y = 11;


The variable y is not needed in the rvalue() test case.

michaelwoerister · 2016-11-07T19:29:33Z

Alright, looking good now. Let's wait for travis to give the OK, then I'll approve.

michaelwoerister · 2016-11-08T16:06:48Z

@bors r+ rollup

bors · 2016-11-08T16:06:51Z

📌 Commit 5a2997d has been approved by michaelwoerister

@michaelwoerister

…r=michaelwoerister Add unary and binary tests for incr-comp This is my draft of tests for unary and binary expressions as desired by rust-lang#37520 for use in the test suite for hashes in incremental compilation. Feedback would be wonderful, if there's any changes I need to make I would appreciate the code review. ?r @michaelwoerister

Rollup of 8 pull requests - Successful merges: #35102, #37425, #37483, #37588, #37601, #37610, #37650, #37652 - Failed merges:

Add unary and binary tests for incr-comp

21814a5

rust-highfive assigned alexcrichton Nov 6, 2016

rust-highfive assigned michaelwoerister and unassigned alexcrichton Nov 6, 2016

michaelwoerister suggested changes Nov 7, 2016

View reviewed changes

oldmanmike added 2 commits November 7, 2016 12:42

Apply changes recommended in code review

e2ca47a

Said code review and recommendations can be found here: rust-lang#37610

Fix typos and redundant code

5a2997d

michaelwoerister reviewed Nov 7, 2016

View reviewed changes

steveklabnik mentioned this pull request Nov 8, 2016

Rollup of 8 pull requests #37657

Merged

bors added a commit that referenced this pull request Nov 9, 2016

Auto merge of #37657 - steveklabnik:rollup, r=steveklabnik

02aa428

Rollup of 8 pull requests - Successful merges: #35102, #37425, #37483, #37588, #37601, #37610, #37650, #37652 - Failed merges:

bors merged commit 5a2997d into rust-lang:master Nov 9, 2016

michaelwoerister mentioned this pull request Nov 10, 2016

incr.comp.: Create Test Case for Incr. Comp. Hash for unary and binary expressions #37520

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add unary and binary tests for incr-comp #37610

Add unary and binary tests for incr-comp #37610

zimurgh commented Nov 6, 2016

rust-highfive commented Nov 6, 2016

alexcrichton commented Nov 6, 2016

michaelwoerister left a comment

michaelwoerister Nov 7, 2016

michaelwoerister Nov 7, 2016

michaelwoerister Nov 7, 2016

michaelwoerister Nov 7, 2016

michaelwoerister Nov 7, 2016

michaelwoerister Nov 7, 2016

michaelwoerister Nov 7, 2016

michaelwoerister Nov 7, 2016

michaelwoerister Nov 7, 2016

michaelwoerister left a comment

michaelwoerister Nov 7, 2016

michaelwoerister Nov 7, 2016

michaelwoerister Nov 7, 2016

michaelwoerister commented Nov 7, 2016

michaelwoerister commented Nov 8, 2016

bors commented Nov 8, 2016

Add unary and binary tests for incr-comp #37610

Add unary and binary tests for incr-comp #37610

Conversation

zimurgh commented Nov 6, 2016

rust-highfive commented Nov 6, 2016

alexcrichton commented Nov 6, 2016

michaelwoerister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelwoerister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelwoerister commented Nov 7, 2016

michaelwoerister commented Nov 8, 2016

bors commented Nov 8, 2016