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

Graphviz implementation #508

Merged
merged 1 commit into from
Feb 16, 2017
Merged

Graphviz implementation #508

merged 1 commit into from
Feb 16, 2017

Conversation

impowski
Copy link
Contributor

This will solve #484 . Right now it's really basic and I will change some of things in future commits like docs and other things.

r? @fitzgen

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks!

Just a few nitpicks below; once they're addressed, I'll r+ and we can merge this :)

Another great follow up would be some docs on how to use this and end up with a png in the CONTRIBUTING.md file.

@@ -2499,6 +2499,14 @@ pub fn codegen(context: &mut BindgenContext) -> Vec<P<ast::Item>> {
}
}

if let Some(path) = context.options().emit_ir_graphviz.as_ref() {
println!("GRAPHVIZ CALLED!!!!");
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: remove debugging prints

println!("GRAPHVIZ CALLED!!!!");
match context.emit_ir_graphviz(path.clone()) {
Ok(()) => println!("Cool!"),
Err(e) => panic!("{}", e),
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: it is probably OK to continue with bindings generation even if writing the dot file fails. We should log an error!(...) with a helpful diagnostic message, which will go to stderr and not get mixed in with the generated bindings.

/// Output graphviz dot file.
pub fn emit_ir_graphviz(&self, path: String) -> io::Result<()> {
let file = try!(File::create(path));
let mut dot_file = Box::new(io::BufWriter::new(file)) as Box<io::Write>;
Copy link
Member

Choose a reason for hiding this comment

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

Why does this need to be boxed? If it doesn't need to be, then we shouldn't box it.

@@ -22,6 +24,19 @@ pub enum ItemKind {
Var(Var),
}

impl fmt::Display for ItemKind {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should implement Display for ItemKind. I think this would be better as a method ItemKind::kind_name(&self) -> &'static str.

@fitzgen
Copy link
Member

fitzgen commented Feb 15, 2017

The reason this is failing in CI is that it needs a rebase:

error: no method named `collect_types` found for type `&ir::item::Item` in the current scope
    --> src/ir/context.rs:1124:18
     |
1124 |             item.collect_types(self, &mut edges, &());
     |                  ^^^^^^^^^^^^^
error: aborting due to previous error

I refactored the graph traversal stuff, and landed it without thinking about how it would affect this PR, sorry! Hopefully the newly refactored stuff is easier to use, but I do feel bad that I bitrotted you out of nowhere.

I think the bit that uses the TypeCollector trait (whose spiritual successor is ir::traversal::Trace) to collect an items edges can become something like this:

item.trace(ctx, &mut |sub_item, _edge_kind| {
    println!("{} -> {};", item.0, sub_item.0);
}, &());

CONTRIBUTING.md Outdated
@@ -112,6 +113,27 @@ Then verify the new Rust bindings compile and pass some basic tests:
$ cargo test -p tests_expectations
```

## Generating graphviz dot file
Copy link
Member

Choose a reason for hiding this comment

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

Nittiest of nit picks, but: This Should Be in Title Case. Also s/file/files/.

Generating Graphviz Dot Files

Copy link
Member

Choose a reason for hiding this comment

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

Also, thanks for documenting this! I think this will help new contributors a ton :)

Another cool thing might be to commit a png generated this way, and show it at the bottom of this section.

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

Perfect! Thank you @impowski!

@fitzgen
Copy link
Member

fitzgen commented Feb 15, 2017

@bors-servo r+

@bors-servo
Copy link

📌 Commit da54412 has been approved by fitzgen

@bors-servo
Copy link

⌛ Testing commit da54412 with merge ac49717...

bors-servo pushed a commit that referenced this pull request Feb 15, 2017
Graphviz implementation

This will solve #484 . Right now it's really basic and I will change some of things in future commits like docs and other things.

r? @fitzgen
@bors-servo
Copy link

☀️ Test successful - status-travis
Approved by: fitzgen
Pushing ac49717 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants