-
Notifications
You must be signed in to change notification settings - Fork 742
Graphviz implementation #508
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
Conversation
There was a problem hiding this 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.
src/codegen/mod.rs
Outdated
@@ -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!!!!"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: remove debugging prints
src/codegen/mod.rs
Outdated
println!("GRAPHVIZ CALLED!!!!"); | ||
match context.emit_ir_graphviz(path.clone()) { | ||
Ok(()) => println!("Cool!"), | ||
Err(e) => panic!("{}", e), |
There was a problem hiding this comment.
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.
src/ir/context.rs
Outdated
/// 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>; |
There was a problem hiding this comment.
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.
src/ir/item_kind.rs
Outdated
@@ -22,6 +24,19 @@ pub enum ItemKind { | |||
Var(Var), | |||
} | |||
|
|||
impl fmt::Display for ItemKind { |
There was a problem hiding this comment.
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
.
The reason this is failing in CI is that it needs a rebase:
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 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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this 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!
@bors-servo r+ |
📌 Commit da54412 has been approved by |
☀️ Test successful - status-travis |
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