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.

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

Suppress MIR comments of FnDef and unit types #75670

Merged
merged 3 commits into from
Aug 20, 2020
Merged

Suppress MIR comments of FnDef and unit types #75670

merged 3 commits into from
Aug 20, 2020

Conversation

tesuji
Copy link
Contributor

@tesuji tesuji commented Aug 18, 2020 •
edited
Loading

An expansion of #75566.
Comments of FnDef MIR constant already contain ty::Contains comments.

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

oli-obk commented Aug 18, 2020

@bors r+

Nice! If you want to do something else along this line, we should really suppress the const infront of FnDefs

@bors
Copy link
Collaborator

bors commented Aug 18, 2020

📌 Commit 0f7a43c861cb07aadcd5efdc328184ec9023a452 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
@tesuji
Copy link
Contributor Author

tesuji commented Aug 18, 2020

@bors r-
Build failed. Will fix in tommorow morning.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 18, 2020
@tesuji tesuji force-pushed the suppress-mir-fndef-ty branch 2 times, most recently from 432fb42 to 7c4d315 Compare August 19, 2020 07:59
tesuji
tesuji commented Aug 19, 2020
View reviewed changes
Copy link
Contributor Author

@tesuji tesuji left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Some tests output are changes. I don't understand why's that?

Edit: Everything is normal. I just read the diff wrong.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 19, 2020

r=me with CI passing

@tesuji
Copy link
Contributor Author

tesuji commented Aug 19, 2020

CI is green.

@tesuji tesuji mentioned this pull request Aug 19, 2020
Merged
@oli-obk
Copy link
Contributor

oli-obk commented Aug 19, 2020

@bors r+

@bors
Copy link
Collaborator

bors commented Aug 19, 2020

📌 Commit 7c4d315 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 19, 2020
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.
@tmandry tmandry mentioned this pull request Aug 19, 2020
Closed
@tmandry
Copy link
Member

tmandry commented Aug 19, 2020

Failed in #75712 (comment), @bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 19, 2020
@tesuji
Copy link
Contributor Author

tesuji commented Aug 20, 2020

mir-opt/instrument_coverage.rs test needs profiler support:

rust/src/test/mir-opt/instrument_coverage.rs

Line 5 in 5f6fcad

// needs-profiler-support

However, profiler is disable by default:

rust/config.toml.example

Line 220 in 5f6fcad

#profiler = false

So the test cannot be run in my machine.

Does profiler need support from LLVM ? I am using local LLVM if that count.

@tesuji tesuji force-pushed the suppress-mir-fndef-ty branch 2 times, most recently from 4cdef08 to 230b70e Compare August 20, 2020 02:30
This was referenced Aug 20, 2020
Closed
Closed
@tesuji tesuji force-pushed the suppress-mir-fndef-ty branch from 230b70e to bb34238 Compare August 20, 2020 09:29
@tesuji tesuji force-pushed the suppress-mir-fndef-ty branch from bb34238 to 86cf461 Compare August 20, 2020 09:33
@tesuji
Copy link
Contributor Author

tesuji commented Aug 20, 2020 •
edited
Loading

Done!

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 20, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Aug 20, 2020

@bors r+ p=1 (let's schedule it before other mir-opt PRs)

@bors
Copy link
Collaborator

bors commented Aug 20, 2020

📌 Commit 86cf461 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 20, 2020
@tesuji tesuji changed the title Suppress MIR comments for FnDef in ty::Const Suppress MIR comments of FnDef and unit types Aug 20, 2020
bjorn3
bjorn3 reviewed Aug 20, 2020
View reviewed changes
src/test/mir-opt/array_index_is_temporary.main.SimplifyCfg-elaborate-drops.after.mir.64bit
@@ -37,9 +37,6 @@ fn main() -> () {
StorageLive(_6); // scope 4 at $DIR/array-index-is-temporary.rs:16:25: 16:26
_6 = _3; // scope 4 at $DIR/array-index-is-temporary.rs:16:25: 16:26
_5 = const foo(move _6) -> bb1; // scope 4 at $DIR/array-index-is-temporary.rs:16:21: 16:27
// ty::Const
// + ty: unsafe fn(*mut usize) -> u32 {foo}
Copy link
Member

Choose a reason for hiding this comment

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

The exact type of functions can sometimes be useful.

Copy link
Contributor

Choose a reason for hiding this comment

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

The type is still shown right below (though we should probably cut out the val part of the literal field as it's useless for zsts.

@bors
Copy link
Collaborator

bors commented Aug 20, 2020

⌛ Testing commit 86cf461 with merge 5fff382...

@bors
Copy link
Collaborator

bors commented Aug 20, 2020

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 20, 2020
@bors bors merged commit 5fff382 into rust-lang:master Aug 20, 2020
@tesuji tesuji deleted the suppress-mir-fndef-ty branch August 20, 2020 14:55
This was referenced Aug 20, 2020
Merged
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 21, 2020
@bors
Auto merge of rust-lang#75697 - lzutao:mir-dumb-const-prefix, r=oli-obk
521db88 
Suppress "const" prefix of FnDef constants in MIR dump

I [was asked][1] to suppress the `const` infront of `FnDef`.
I tried to suppress comments for other types, but turned out that `const ()` and `()` is different: rust-lang#75697 (comment)

[1]: rust-lang#75670 (comment)
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oli-obk oli-obk oli-obk left review comments

@bjorn3 bjorn3 bjorn3 left review comments

Assignees

@oli-obk oli-obk

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.
8 participants
@tesuji @oli-obk @bors @tmandry @bjorn3 @cuviper @rust-highfive @rustbot