webidl: add support for partial interfaces and mixins #460
Conversation
This is a major change to how webidl is processed. This adds a two phase process, where the first phase records the names of various types and indexes the mixins (and might do more in the future). The actual program building happens in the second phase. As part of this, this also makes it so that interface objects are passed by reference, rather than by value. The spec isn't exactly clear on this, but Mozilla's C++ reflection suggestions seem to indicate that they should be passed by reference (see https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings).
crates/webidl/src/lib.rs
Outdated
| } | ||
| } | ||
|
|
||
| impl<'a, 'b> WebidlParse<&'a FirstPass<'b>> for webidl::ast::Interface { |
There was a problem hiding this comment.
I'd personally recommend avoiding the WebidlParse trait entirely as this looks like it may be getting a little too verbose, but otherwise perhaps the FirstPass parameter could be part of the trait signature to avoid the wordiness of lifetimes here?
There was a problem hiding this comment.
+1 for FirstPass as part of the trait signature, since pretty much every impl is using it.
crates/webidl/tests/all/simple.rs
Outdated
| @@ -44,14 +44,14 @@ fn method() { | |||
| let pi = Foo::new(3.14159).unwrap(); | |||
| let e = Foo::new(2.71828).unwrap(); | |||
| // TODO: figure out why the following doesn't fail | |||
There was a problem hiding this comment.
FWIW this should be fixed now in upstream rust if you wanna do a drive-by cleanup as well
|
Seems like a great cleanup to me and a good way to structure it as well! |
There was a problem hiding this comment.
This is a really nice (and overdue!) refactoring! Thanks so much @ohanar :)
I think we can eventually (not in this PR for sure!) also port my hacky is-this-imported-name-defined-or-not analysis to just use FirstPass data, too.
Couple nitpicks I'd like to see addressed before merging, but again this is really great. Thanks!
crates/webidl/src/lib.rs
Outdated
|
|
||
| let mut first_pass = FirstPass::default(); | ||
| for def in definitions { | ||
| if let Definition::Interface(Interface::NonPartial(NonPartialInterface { name, .. })) = def |
There was a problem hiding this comment.
Nitpick: stylistically I'd prefer using a match with a _ arm to a series of if lets.
crates/webidl/src/util.rs
Outdated
| raw_ident(&format!("__widl_f_{}_{}", rust_name, ns)) | ||
| }; | ||
| #[derive(Default)] | ||
| pub struct FirstPass<'a> { |
There was a problem hiding this comment.
I think a doc comment here explaining what the first pass over the WebIDL does, what information is collected, and for what use and why would be very valuable for future code readers and contributors.
crates/webidl/src/util.rs
Outdated
| ) -> Option<backend::ast::ImportFunction> { | ||
| let ret = match self.webidl_ty_to_syn_ty(ty, TypePosition::Return) { | ||
| None => { | ||
| warn!("Attribute's type does not yet support reading: {:?}", ty); |
There was a problem hiding this comment.
Should this warning message s/reading/returning/ ?
There was a problem hiding this comment.
I guess this is how it was before, but the message no longer makes sense to me, and I assume I originally wrote it...
crates/webidl/src/lib.rs
Outdated
| } | ||
| } | ||
|
|
||
| impl<'a, 'b> WebidlParse<&'a FirstPass<'b>> for webidl::ast::Interface { |
There was a problem hiding this comment.
+1 for FirstPass as part of the trait signature, since pretty much every impl is using it.
I also updated it so that it is modeled in the same extensible way as the WebidlParse trait.
|
I ended up moving out the new first pass into a new module, and restructuring the new code a bit to make it more extensible in the future. Let me know if you have any new feedback, I hope to finish up addressing your previous comments tomorrow. |
ghost
mentioned this pull request
Fixes #246 and #255.
This is a major change to how webidl is processed. This adds
a two phase process, where the first phase records the names of
various types and indexes the mixins (and might do more in the
future). The actual program building happens in the second phase.
As part of this, this also makes it so that interface objects
are passed by reference, rather than by value. The spec isn't
exactly clear on this, but Mozilla's C++ reflection suggestions
seem to indicate that they should be passed by reference (see
https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings).