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

Use syn and quote crates for the match_token! macro… #217

Merged
merged 2 commits into from
Oct 18, 2016
Merged

Conversation

SimonSapin
Copy link
Member

@SimonSapin SimonSapin commented Oct 3, 2016

… 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 Reviewable

@@ -33,7 +33,7 @@ harness = false
[features]
unstable = ["tendril/unstable", "string_cache/unstable"]
heap_size = ["heapsize", "heapsize_plugin"]
codegen = ["html5ever_macros"]
codegen = []
Copy link
Member Author

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.

Copy link
Member

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?

Copy link
Member Author

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.
Copy link
Member Author

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
Copy link
Member Author

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.

Copy link
Member Author

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.

Copy link
Member

@emilio emilio left a 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;
Copy link
Member

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?

Copy link
Member Author

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.

@SimonSapin
Copy link
Member Author

Are there any tests for this macro?

Not directly, but there are many tree builder tests that exercise the generated code.

@Ygg01
Copy link
Contributor

Ygg01 commented Oct 5, 2016

How easy will this be the modify?

@nox
Copy link
Contributor

nox commented Oct 11, 2016

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.

@Ygg01
Copy link
Contributor

Ygg01 commented Oct 11, 2016

@nox: syn is for derive macros, only, as far as I can tell. See discussion here: #216

@nox
Copy link
Contributor

nox commented Oct 11, 2016

@Ygg01
Copy link
Contributor

Ygg01 commented Oct 11, 2016

@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 syn? I hate to be an energy vampire, so could you just explain what you meant?

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)
          }
    }
}

@bors-servo
Copy link
Contributor

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

@SimonSapin SimonSapin changed the title Use a hand-written parser a string formatting for the match_token! macro… Use syn and quote crates for the match_token! macro… Oct 17, 2016
@SimonSapin
Copy link
Member Author

Alright! I pushed a rewrite that uses the syn and quote crates, the provide real parser and quasi-quoting.

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.
@nox
Copy link
Contributor

nox commented Oct 18, 2016

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit dd9f2cc has been approved by nox

@bors-servo
Copy link
Contributor

⌛ Testing commit dd9f2cc with merge b8354e4...

bors-servo pushed a commit that referenced this pull request Oct 18, 2016
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 -->
@bors-servo
Copy link
Contributor

☀️ Test successful - status-travis

@bors-servo bors-servo merged commit dd9f2cc into master Oct 18, 2016
@SimonSapin SimonSapin deleted the diy-macros branch October 18, 2016 11:53
SimonSapin added a commit to SimonSapin/syn that referenced this pull request Oct 20, 2016
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).
SimonSapin added a commit to SimonSapin/syn that referenced this pull request Oct 20, 2016
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).
dtolnay pushed a commit to dtolnay/syn that referenced this pull request Oct 20, 2016
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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Macros to generate tree building rules don't work in current Rust
5 participants