-
Notifications
You must be signed in to change notification settings - Fork 231
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
Use syn and quote crates for the match_token!
macro…
#217
Conversation
@@ -33,7 +33,7 @@ harness = false | |||
[features] | |||
unstable = ["tendril/unstable", "string_cache/unstable"] | |||
heap_size = ["heapsize", "heapsize_plugin"] | |||
codegen = ["html5ever_macros"] | |||
codegen = [] |
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.
This feature is now unused, but removing it would be a breaking change. So let’s keep it until we have some other reason to make a breaking change.
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.
Can you add a comment with that in the file so we don't accidentally bump the version without removing the feature?
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.
Done.
@@ -7,7 +7,7 @@ | |||
// option. This file may not be copied, modified, or distributed | |||
// except according to those terms. | |||
|
|||
//! The tree builder rules, as a single, enormous nested match expression. |
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.
include!
doesn’t like top-level attributes.
@@ -770,7 +770,7 @@ impl<Handle, Sink> TreeBuilderStep | |||
Done | |||
} | |||
|
|||
// FIXME: This should be unreachable, but match_token! requires a | |||
// FIXME: This should be unreachable, but match_token requires a |
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.
Yes, the parser is that stupid. It would find match_token!
in a comment and try to parse it as a macro invocation.
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.
Note: this comment referred to my hand-written parser. syn
doesn’t have this problem.
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.
Are there any tests for this macro?
} | ||
|
||
pub fn expand_match_tokens(from: &Path, to: &Path) { | ||
// use std::fmt::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.
Do you plan to keep these comments for any specific reasons? Or is it fine to remove them?
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.
They were temporary, for debugging. I forgot to remove them before committing. Removed now.
Not directly, but there are many tree builder tests that exercise the generated code. |
How easy will this be the modify? |
syn 0.9.0 is out with many improvements, could the hand-written parser use it? I'm really wary of merging something that can also end up transforming comments. |
@nox: I'm a newbie when it comes to procedural macros, but those notes don't show me anything that would make full procedural macros parsable by Did you mean we could parse tags as: match mode {
InitialMode =>
#[derive(tags)]
match token {
CharacterTokens(NotSplit, text) => SplitWhitespace(text),
...
token => {
if !self.opts.iframe_srcdoc {
self.unexpected(&token);
self.set_quirks_mode(Quirks);
}
Reprocess(BeforeHtml, token)
}
}
} |
☔ The latest upstream changes (presumably #218) made this pull request unmergeable. Please resolve the merge conflicts. |
match_token!
macro…match_token!
macro…
Alright! I pushed a rewrite that uses the r? @nox I’ll rebase and fix conflicts tomorrow. |
…macro… … instead of the parser and quasi-quoting from Rust’s (unstable) libsyntax. This has significantly worse diagnostics when encountering unexpected syntax (e.g. no indication of which line has the offending code) but this removes all usage of unstable compiler internals that constantly need to be fixed when updating the compiler. Fixes #216.
548a0d5
to
63bf17f
Compare
63bf17f
to
dd9f2cc
Compare
@bors-servo r+ |
📌 Commit dd9f2cc has been approved by |
Use syn and quote crates for the `match_token!` macro… … instead of the parser and quasi-quoting from Rust’s (unstable) libsyntax. This has significantly worse diagnostics when encountering unexpected syntax (e.g. no indication of which line has the offending code) but this removes all usage of unstable compiler internals that constantly need to be fixed when updating the compiler. Fixes #216. r? @nox <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/html5ever/217) <!-- Reviewable:end -->
☀️ Test successful - status-travis |
Token trees are much simpler than an AST. This makes then easier to work with when they are sufficent for the task at hand, such as for example [expanding a procedural macro]( servo/html5ever#217).
Token trees are much simpler than an AST. This makes them easier to work with when they are sufficient for the task at hand, such as for example [expanding a procedural macro]( servo/html5ever#217).
Token trees are much simpler than an AST. This makes them easier to work with when they are sufficient for the task at hand, such as for example [expanding a procedural macro]( servo/html5ever#217).
… instead of the parser and quasi-quoting from Rust’s (unstable) libsyntax.
This has significantly worse diagnostics when encountering unexpected syntax (e.g. no indication of which line has the offending code) but this removes all usage of unstable compiler internals that constantly need to be fixed when updating the compiler.
Fixes #216.
r? @nox
This change is