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

Update fingerprint tests macros #46523

Merged
merged 8 commits into from Dec 7, 2017

Conversation

Projects
None yet
5 participants
@CrockAgile
Copy link
Contributor

CrockAgile commented Dec 5, 2017

CrockAgile added some commits Dec 5, 2017

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 5, 2017

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

@CrockAgile

This comment has been minimized.

Copy link
Contributor Author

CrockAgile commented Dec 5, 2017

Between these and the previous updates I contributed, I gained decent deduction skills as to which artifacts (HirBody/MirValidated/etc) should change in response to certain code changes. This indicates to me that tagging mentor issues is working 👍

I did encounter one issue which confused my intuition and sparked a bit of curiosity. while_let_loops.rs and while_loops.rs test a common case of changing a continue's label, but the dirty fingerprints are different:

// while_let_loops.rs
#[cfg(cfail1)]
pub fn change_continue_label() {
    let mut _x = 0;
    'outer: while true {
        'inner: while true {
            _x = 1;
            continue 'inner;
        }
    }
}

#[cfg(not(cfail1))]
#[rustc_clean(cfg="cfail2", except="HirBody, MirValidated")]
#[rustc_clean(cfg="cfail3")]
pub fn change_continue_label() {
    let mut _x = 0;
    'outer: while true {
        'inner: while true {
            _x = 1;
            continue 'outer;
        }
    }
}

// while_let_loops.rs
#[cfg(cfail1)]
pub fn change_continue_label() {
    let mut _x = 0;
    'outer: while let Some(0u32) = None {
        'inner: while let Some(0u32) = None {
            _x = 1;
            continue 'inner;
        }
    }
}

#[cfg(not(cfail1))]
#[rustc_clean(cfg="cfail2", except="HirBody, MirValidated, MirOptimized, TypeckTables")]
#[rustc_clean(cfg="cfail3")]
pub fn change_continue_label() {
    let mut _x = 0;
    'outer: while let Some(0u32) = None {
        'inner: while let Some(0u32) = None {
            _x = 1;
            continue 'outer;
        }
    }
}

In the "while loop" case, only HirBody and MirValidated are dirty. It is a little strange that only MirValidated is dirty and not MirOptimized, but I assume this is because optimization gives identical results regardless of label.

But if this is the case for "while loops", why is it not for "while let loops"? Instead, "while let loops" give a dirty MirOptimized as well as TypeckTables. This may not be important, but since it was the only bit of code which gave me unintuitive results I thought I should bring it up.

@michaelwoerister

This comment has been minimized.

Copy link
Contributor

michaelwoerister commented Dec 6, 2017

Wow, thanks a lot, @CrockAgile! I'll review as soon as I find the time.

I did encounter one issue which confused my intuition and sparked a bit of curiosity.

That's curious indeed. However looking at the MIR on play.rust-lang.org, both versions are the same for the vanilla while case and we get a warning that it is an infinite loop. I suspect that some optimization pass merges the two loops. The code generated from while let is a lot more complicated an the optimization doesn't kick in.

@michaelwoerister

This comment has been minimized.

Copy link
Contributor

michaelwoerister commented Dec 7, 2017

Looks good to me. Thanks, @CrockAgile!

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 7, 2017

📌 Commit 3f0cc7c has been approved by michaelwoerister

@michaelwoerister

This comment has been minimized.

Copy link
Contributor

michaelwoerister commented Dec 7, 2017

@bors p=1

bors added a commit that referenced this pull request Dec 7, 2017

Auto merge of #46523 - CrockAgile:update-fingerprint-tests-macros, r=…
…michaelwoerister

Update fingerprint tests macros

Part of #44924

r? @michaelwoerister
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 7, 2017

⌛️ Testing commit 3f0cc7c with merge ee25791...

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 7, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: michaelwoerister
Pushing ee25791 to master...

@bors bors merged commit 3f0cc7c into rust-lang:master Dec 7, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@CrockAgile CrockAgile deleted the CrockAgile:update-fingerprint-tests-macros branch Dec 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.