make MIR graphviz generation use gsgdt by vn-ki · Pull Request #78399 · rust-lang/rust

vn-ki · 2020-10-26T13:39:21Z

gsgdt [https://crates.io/crates/gsgdt] is a crate which provides an
interface for stringly typed graphs. It also provides generation of
graphviz dot format from said graph.

This is the first in a series of PRs on moving graphviz code out of rustc into normal crates and then implementating graph diffing on top of these crates.

r? @oli-obk

rust-highfive · 2020-10-26T13:39:23Z

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

oli-obk

cc @rust-lang/wg-mir-opt

bors · 2020-11-06T09:44:37Z

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

richkadel · 2020-11-06T16:27:38Z

@vn-ki - I just saw this PR was mentioned after bors landed a conflicting commit from my PR #78267 . I like your goals of moving graphviz code out of rustc. Note that my PR added a generic_graphviz.rs implementation, based mostly on the MIR graphviz.rs in the same directory. I needed a MIR-like graphviz output for coverage graphs, but didn't want to create another type-specific 1-off.

You may need to look that over and decide how to work in your changes, whether replacing my implementation or not. FYI, I implemented enough hooks (by generic handler functions) to support the coverage graph, but not all of the hooks required to completely implement an equivalent to the existing MIR graphviz.rs implementation. It wouldn't be hard to extend generic_graphviz.rs to do that though. But you may or may not want to consider that approach. (If this is even relevant to your change.)

gsgdt [https://crates.io/crates/gsgdt] is a crate which provides an interface for stringly typed graphs. It also provides generation of graphviz dot format from said graph.

vn-ki · 2020-11-09T17:16:15Z

This PR does not intend to touch coverage graphs, so I guess both implementations will be in rustc (for now).

I will move the rest of the graphviz consumers to gsgdt in future PRs, and then we will be able to finally remove all graphviz code.

Most of the current logic in mir::util::graphviz has been ported to gsgdt because I thought that would be a general approach that will be required for most graphs of such nature. So I think we won't be needing generic_graphviz, though I'm not sure.

vn-ki · 2020-11-09T17:16:26Z

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

oli-obk · 2020-11-10T07:40:52Z

@bors r+

bors · 2020-11-10T07:40:53Z

📌 Commit 86a7831 has been approved by oli-obk

bors · 2020-11-10T07:40:54Z

🌲 The tree is currently closed for pull requests below priority 100, this pull request will be tested once the tree is reopened

richkadel · 2020-11-11T15:47:42Z

+ let label = node(def_id, block);
+
+ let (title, bgcolor) = if data.is_cleanup {
+ (format!("{} (cleanup)", block.index()), "lightblue")


I just noticed you missed a bug fix during the rebase conflict resolution:
This should be:
let color = if dark_mode {"royalblue"}else{"lightblue"};(format!("{} (cleanup)", block.index()), color)
See line 116 in the deleted sections of graphviz.rs

oli-obk · 2020-11-11T16:08:42Z

@bors r-

bors · 2020-12-04T20:07:00Z

💔 Test failed - checks-actions

camelid · 2020-12-04T21:30:08Z

@rust-lang/infra bors seems to have tried to merge this PR even though it shouldn't have been in the approved state. That, or someone r='d and then deleted their comment?

camelid · 2020-12-04T21:30:49Z

Also, Oli's @bors r- didn't seem to work.

camelid · 2020-12-04T21:31:33Z

Should I have cc'd Pietro instead? I know they work on bors, and they don't seem to be on the infra GH team...

Mark-Simulacrum · 2020-12-04T21:35:20Z

Pietro is absolutely on the infra GH team and you should always ping the team. I think once bors starts testing you need to r- retry.

camelid · 2020-12-04T21:38:21Z

Pietro is absolutely on the infra GH team and you should always ping the team.

Ah, GitHub was only showing three team members in the hovercard, so it confused me.

I think once bors starts testing you need to r- retry.

Hmm, maybe that should be changed so just an r- works? It seems like somewhat unintuitive behavior.

ebroto · 2020-12-05T01:02:37Z

I also hit this.

TL;DR I think you can hotfix this by adapting the test, the changes should not affect what is being tested.

diff --git a/src/tools/clippy/tests/ui/crashes/used_underscore_binding_macro.rs b/src/tools/clippy/tests/ui/crashes/used_underscore_binding_macro.rs
index 6d2124c12fe..c57a45dc7aa 100644
--- a/src/tools/clippy/tests/ui/crashes/used_underscore_binding_macro.rs+++ b/src/tools/clippy/tests/ui/crashes/used_underscore_binding_macro.rs@@ -1,10 +1,9 @@-#![allow(clippy::useless_attribute)] //issue #2910+// edition:2018-#[macro_use]-extern crate serde_derive;+use serde::Deserialize;
/// Tests that we do not lint for unused underscores in a `MacroAttribute`
/// expansion
#[deny(clippy::used_underscore_binding)]
#[derive(Deserialize)]
struct MacroAttributesTest {

Long story: Clippy tries to avoid this problem by adding --extern flags for crates that are also in the sysroot. In the rustc repo, two different deps dirs are passed (with -L) when compiling the test, build/x86_64-unknown-linux-gnu/stage1-tools/release/deps and build/x86_64-unknown-linux-gnu/stage1-tools/x86_64-unknown-linux-gnu/release/deps, but Clippy is only checking the last one. Moreover, only rlibs are taken into account, and serde_derive is a .so. Checking both dirs is also not an option, because in that case two instances of quote are found.

This is a long-standing issue in Clippy, which would be fixed by knowing the exact libraries cargo used when building clippy_lints. I will try to see what I can do to solve the general problem.

Dylan-DPC-zz · 2020-12-05T16:50:29Z

@oli-obk this is ready for review

oli-obk · 2020-12-15T09:08:04Z

@bors r+

bors · 2020-12-15T09:08:06Z

📌 Commit 6fe31e7 has been approved by oli-obk

make MIR graphviz generation use gsgdt gsgdt [https://crates.io/crates/gsgdt] is a crate which provides an interface for stringly typed graphs. It also provides generation of graphviz dot format from said graph. This is the first in a series of PRs on moving graphviz code out of rustc into normal crates and then implementating graph diffing on top of these crates. r? `@oli-obk`

make MIR graphviz generation use gsgdt gsgdt [https://crates.io/crates/gsgdt] is a crate which provides an interface for stringly typed graphs. It also provides generation of graphviz dot format from said graph. This is the first in a series of PRs on moving graphviz code out of rustc into normal crates and then implementating graph diffing on top of these crates. r? ``@oli-obk``

make MIR graphviz generation use gsgdt gsgdt [https://crates.io/crates/gsgdt] is a crate which provides an interface for stringly typed graphs. It also provides generation of graphviz dot format from said graph. This is the first in a series of PRs on moving graphviz code out of rustc into normal crates and then implementating graph diffing on top of these crates. r? ```@oli-obk```

bors · 2020-12-15T22:00:08Z

⌛ Testing commit 6fe31e7 with merge 4031f7b...

bors · 2020-12-16T00:47:44Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 4031f7b to master...

make MIR graphviz generation use gsgdt gsgdt [https://crates.io/crates/gsgdt] is a crate which provides an interface for stringly typed graphs. It also provides generation of graphviz dot format from said graph. This is the first in a series of PRs on moving graphviz code out of rustc into normal crates and then implementating graph diffing on top of these crates. r? `@oli-obk`

rust-highfive assigned oli-obk Oct 26, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 26, 2020

oli-obk reviewed Oct 26, 2020
View reviewed changes

Comment threadcompiler/rustc_mir/src/util/graphviz.rs Outdated

vn-ki force-pushed the gsgdt-graphviz branch 2 times, most recently from a09a9f8 to d5bc68eCompare October 26, 2020 16:22

vn-ki mentioned this pull request Oct 28, 2020
Move graphviz code out of the compiler into external crate rust-lang/compiler-team#380
Closed
3 tasks

oli-obk added S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 3, 2020

bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Nov 6, 2020

vn-ki added 3 commits November 9, 2020 22:39

make MIR graphviz generation use gsgdt
ea14607
gsgdt [https://crates.io/crates/gsgdt] is a crate which provides an interface for stringly typed graphs. It also provides generation of graphviz dot format from said graph.

write to a String instead to reduce churn
5b049e1

update gsgdt
a4e94ec

vn-ki force-pushed the gsgdt-graphviz branch from 414414b to a4e94ecCompare November 9, 2020 17:12

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 Nov 9, 2020

vn-ki commented Nov 9, 2020
View reviewed changes

Comment threadCargo.lock Outdated

formatting
86a7831

vn-ki force-pushed the gsgdt-graphviz branch from 63b896b to 86a7831Compare November 9, 2020 18:51

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 Nov 10, 2020

richkadel suggested changes Nov 11, 2020
View reviewed changes

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-review Status: Awaiting review from the assignee but also interested parties. labels Dec 4, 2020

bors 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 Dec 4, 2020

fix clippy test
6fe31e7

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 Dec 15, 2020

Dylan-DPC-zz mentioned this pull request Dec 15, 2020
Rollup of 10 pull requests #80061
Closed

This was referenced Dec 15, 2020
Rollup of 10 pull requests #80063
Closed
Rollup of 11 pull requests #80064
Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 16, 2020

bors merged commit 4031f7b into rust-lang:masterDec 16, 2020

rustbot added this to the 1.50.0 milestone Dec 16, 2020

Uh oh!

Uh oh!

Conversation

vn-ki commented Oct 26, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Oct 26, 2020

Uh oh!

oli-obk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bors commented Nov 6, 2020

Uh oh!

richkadel commented Nov 6, 2020

Uh oh!

vn-ki commented Nov 9, 2020

Uh oh!

vn-ki commented Nov 9, 2020

Uh oh!

Uh oh!

oli-obk commented Nov 10, 2020

Uh oh!

bors commented Nov 10, 2020

Uh oh!

bors commented Nov 10, 2020

Uh oh!

richkadelNov 11, 2020

Choose a reason for hiding this comment

Uh oh!

oli-obk commented Nov 11, 2020

Uh oh!

bors commented Dec 4, 2020

Uh oh!

camelid commented Dec 4, 2020

Uh oh!

camelid commented Dec 4, 2020

Uh oh!

camelid commented Dec 4, 2020

Uh oh!

Mark-Simulacrum commented Dec 4, 2020

Uh oh!

camelid commented Dec 4, 2020

Uh oh!

ebroto commented Dec 5, 2020

Uh oh!

Dylan-DPC-zz commented Dec 5, 2020

Uh oh!

oli-obk commented Dec 15, 2020

Uh oh!

bors commented Dec 15, 2020

Uh oh!

bors commented Dec 15, 2020

Uh oh!

bors commented Dec 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

vn-ki commented Oct 26, 2020•
edited
Loading