Skip to content

Suppress verbose MIR comments for trivial types #75566

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Merged
merged 2 commits into from
Aug 18, 2020
Merged

Suppress verbose MIR comments for trivial types #75566

merged 2 commits into from
Aug 18, 2020

Conversation

alasher
Copy link
Contributor

@alasher alasher commented Aug 15, 2020

Addresses #74508

This is my first contribution to the Rust project! Please let me know if anything needs revising, I'm happy to make changes.

@rust-highfive
Copy link
Contributor

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 15, 2020
@lcnr
Copy link
Contributor

lcnr commented Aug 15, 2020

👍 r? @oli-obk

You still have to run ./x.py test src/test/mir-opt --stage 1 --bless to update the test outputs which use pretty printing.

The same also goes for 32 bit mir tests, let me quickly find the command for that again 😅

@lcnr
Copy link
Contributor

lcnr commented Aug 15, 2020

For the remaining test I think you can use ./x.py test src/test/mir-opt/ --bless --stage 1 --target i686-unknown-linux-gnu. You might have to install the multilib version of your toolchain to do so.

@alasher
Copy link
Contributor Author

alasher commented Aug 15, 2020

Thanks for the command to run the tests, I figured I'd run into trouble with those but didn't know how to run them. 😄 I've rebased the modified test files into the same commit, let me know if they should be split. I'll reupdate the test files if necessary after review.

@lcnr
Copy link
Contributor

lcnr commented Aug 15, 2020

I think keeping them split would be useful, as the test changes are quite large 🤔

Didn't look too much at the tests yet, but that's already looking a lot better IMO

tesuji
tesuji reviewed Aug 16, 2020
View reviewed changes
@alasher
Copy link
Contributor Author

alasher commented Aug 16, 2020

@lzutao Addressed your feedback + rebased

@oli-obk
Copy link
Contributor

oli-obk commented Aug 17, 2020

@bors r+ p=6 bitrotty

Thanks! so much better

@bors
Copy link
Collaborator

bors commented Aug 17, 2020

📌 Commit 64db07dba347eb91facb232dee5794e95d8e1e77 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 Aug 17, 2020
@bors
Copy link
Collaborator

bors commented Aug 17, 2020

⌛ Testing commit 64db07dba347eb91facb232dee5794e95d8e1e77 with merge bdbd20ab6913e3b3ba18b820fda7511ae16f1bdb...

@bors
Copy link
Collaborator

bors commented Aug 17, 2020

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 17, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Aug 17, 2020

Oh no... instrument_coverage failed. In order to run that test you need to enable the profiler flag:

rust/config.toml.example

Line 220 in e8df0b8

#profiler = false

@alasher
Copy link
Contributor Author

alasher commented Aug 17, 2020

@oli-obk Alright, I think we're good now! instrument_coverage has been updated, unless there's another testcase hiding somewhere we should be good :) I rg'd the whole mir-opt folder and couldn't find anything else matching the now-removed output.

@bors
Copy link
Collaborator

bors commented Aug 17, 2020

☔ The latest upstream changes (presumably #74748) made this pull request unmergeable. Please resolve the merge conflicts.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 18, 2020

@bors r+ p=6

@bors
Copy link
Collaborator

bors commented Aug 18, 2020

📌 Commit 28ac141 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 Aug 18, 2020
@bors
Copy link
Collaborator

bors commented Aug 18, 2020

⌛ Testing commit 28ac141 with merge 4717cf2...

@bors
Copy link
Collaborator

bors commented Aug 18, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: oli-obk
Pushing 4717cf2 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 18, 2020
@bors bors merged commit 4717cf2 into rust-lang:master Aug 18, 2020
This was referenced Aug 18, 2020
Merged
Merged
@tesuji tesuji mentioned this pull request Aug 18, 2020
Merged
tmandry added a commit to tmandry/rust that referenced this pull request Aug 19, 2020
@tmandry
Rollup merge of rust-lang#75670 - lzutao:suppress-mir-fndef-ty, r=oli…
5256b95 
…-obk

Suppress MIR comments for FnDef in ty::Const

An expansion of rust-lang#75566.
The comments in MIR constant already contains `ty::Contains` comments.
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 20, 2020
@bors
Auto merge of rust-lang#75670 - lzutao:suppress-mir-fndef-ty, r=oli-obk
5fff382 
Suppress MIR comments of FnDef and unit types

An expansion of rust-lang#75566.
Comments of FnDef MIR constant already contain `ty::Contains` comments.
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@tesuji tesuji tesuji left review comments

Assignees

@oli-obk oli-obk

@lcnr lcnr

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.47.0
Development

Successfully merging this pull request may close these issues.
7 participants
@alasher @rust-highfive @lcnr @oli-obk @bors @tesuji @cuviper