Refactor to keep up with changes to proc_macro by anxiousmodernman · Pull Request #122 · lambda-fairy/maud

anxiousmodernman · 2018-04-09T19:06:11Z

Instead of a kind field containting a TokenNode variant, a
TokenTree is now an enum with variants of different types (Literal, Op,
Term, etc). Note that a TokenTree could be a sequence of TokenTrees if it is a
Group variant.

Other notes:

I'm unsure about the set_span call in Builder::emit_if, but I did not want to
throw away the passed in Span.

Parsing relies on destructuring references to the values associated with
TokenTree enum variants

Refs #121

anxiousmodernman · 2018-04-09T19:10:41Z

    quote!({
        extern crate maud;
-        let mut $output_ident = String::with_capacity($size_hint as usize);
+        //let mut $output_ident = String::with_capacity($size_hint as usize);


The compiler complains about $size_hint here

anxiousmodernman · 2018-04-09T19:25:10Z

@lfairy This compiles, but I have introduced some parsing errors in the process. My hope is that we can work through these and keep the broad structure of the code intact.
I do reproduce these test failures locally.

lambda-fairy · 2018-04-10T04:55:40Z

I don't have time to look at this yet, but if you change the html! calls in the failing tests to html_debug! then the generated code will be logged to the console. From there you should be able to spot the problem.

lambda-fairy · 2018-04-10T09:55:54Z

+            tts.extend(i);
+        }
+
+        tts.into_iter().collect()


I believe this change can be reverted once rust-lang/rust#49734 lands

lambda-fairy · 2018-04-10T10:02:43Z


    fn match_arms(&mut self) -> ParseResult<TokenStream> {
-        let mut arms = Vec::new();
+        let mut arms: Vec<TokenTree> = Vec::new();


rust-lang/rust#49734 applies here as well

anxiousmodernman · 2018-04-10T19:43:15Z

Life on the bleeding edge! 🐎 💨

anxiousmodernman · 2018-04-12T20:49:42Z

I think we're close. Tests pass for me locally, but on CI the hyper dependency doesn't compile. I'll clean up warnings and try to work it out.

There are newer releases of hyper: https://github.com/hyperium/hyper/releases

lambda-fairy · 2018-04-12T20:54:31Z

Clippy failing on Hyper is a known issue (rust-lang/rust#49643; there's a Clippy issue as well but I can't find it right now). Since it's not our fault I think we can ignore it for now 🙂

anxiousmodernman · 2018-04-12T21:03:59Z

Alrighty. I'll squash these down.

Instead of a `kind` field containting a `TokenNode` variant, a TokenTree is now an enum with variants of different types (Literal, Op, Term, etc). Note that a TokenTree could be a sequence of TokenTrees if it is a Group variant. Other notes: I'm unsure about the set_span call in Builder::emit_if, but I did not want to throw away the passed in Span. Parsing relies on destructuring references to the values associated with TokenTree enum variants It doesn't seem as easy to compose/chain TokenStreams as it is to collect a Vec<TokenTree> into a TokenStream. There is probably some iterator API I could use instead. See `match_arms` and build.rs Refs lambda-fairy#121

anxiousmodernman · 2018-04-12T21:30:03Z

rust-lang/rust#49734 was merged 21 hours ago. I think we might need to wait another day before it's in nightly.

Also remove conservative_impl_trait feature flag, as this is now a stable feature. Refs lambda-fairy#121

anxiousmodernman · 2018-04-16T08:12:52Z

I've addressed the previous comments now that the iterator changes have landed.

lambda-fairy

Thanks for sticking through with this!

I have a few comments to make, but as they're pretty minor I'll merge your changes as-is and fix it up myself.

lambda-fairy · 2018-04-16T07:06:16Z

-            });
+            let mut g = Group::new(Delimiter::Parenthesis, cond);
+            // NOTE: Do we need to do this?
+            g.set_span(cond_span);


To be honest I'm not actually sure 😛

My original thought was that using the source spans more often would improve compiler diagnostics. But since we're creating these parentheses out of whole cloth it's a bit questionable to use them here.

I'm in the process of rewriting this file in another branch, so I'll address this point there.

lambda-fairy · 2018-04-16T07:09:50Z

-        if cond.clone().into_iter().any(|token| match token.kind {
-            TokenNode::Group(Delimiter::Brace, _) => true,
+        if cond.clone().into_iter().any(|token| match token {
+            TokenTree::Group(grp) => { 


In general I tend to avoid minor abbreviations like this.

cond is okay because condition is pretty long. But group vs grp isn't such a clear win.

(I realize I abbreviate pattern to pat elsewhere... should probably fix that)

lambda-fairy · 2018-04-16T07:19:56Z

    /// Indicates whether we're inside an attribute node.
    in_attr: bool,
-    input: TokenTreeIter,
+    input: token_stream::IntoIter,


<TokenStream as IntoIterator>::IntoIter will avoid the import (as well as signal our intentions more clearly)

lambda-fairy · 2018-04-16T07:25:22Z

                    // When emitting a `@let`, wrap the rest of the block in a
                    // new block to avoid scoping issues
-                    let keyword = TokenTree { kind: TokenNode::Term(term), span };
+                    let keyword = Term::new(term.as_str(), term.span());


The old code did this term wrapping dance because the identifier and span used to be in separate fields, such that matching the term required pulling it apart and putting it back together again.

But now that the Term data type contains its own span, we can use the original term without the dance.

lambda-fairy · 2018-04-16T07:37:51Z

+                                let block = self.block(grp.stream(), grp.span())?;
+                                builder.push(block);
+                            },
+                            _ => { return self.error("expected body for @else"); },


return is an expression -- you can omit the braces if it's the only thing in the block:

_ => return self.error("..."),

lambda-fairy · 2018-04-16T08:07:13Z

-            kind: TokenNode::Group(Delimiter::Brace, builder.build()),
-            span,
-        })
+        Ok(TokenTree::Group(Group::new(Delimiter::Brace, builder.build())))


This code should call .set_span() on the generated Group

anxiousmodernman commented Apr 9, 2018

View reviewed changes

lambda-fairy reviewed Apr 10, 2018

View reviewed changes

anxiousmodernman force-pushed the 121_proc_macro branch from aee31d4 to 716824a Compare April 12, 2018 21:05

anxiousmodernman changed the title ~~WIP: Refactor to keep up with changes to proc_macro~~ Refactor to keep up with changes to proc_macro Apr 12, 2018

anxiousmodernman force-pushed the 121_proc_macro branch from 716824a to 0f453a5 Compare April 12, 2018 21:21

anxiousmodernman added 2 commits April 16, 2018 00:54

Use FromIterator<_> for TokenStream

2d6558c

Also remove conservative_impl_trait feature flag, as this is now a stable feature. Refs lambda-fairy#121

Pre-allocate sufficient size for our output String

f75da83

lambda-fairy approved these changes Apr 16, 2018

View reviewed changes

lambda-fairy merged commit f75da83 into lambda-fairy:master Apr 16, 2018

Conversation

anxiousmodernman commented Apr 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anxiousmodernman commented Apr 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lambda-fairy commented Apr 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anxiousmodernman commented Apr 10, 2018

Uh oh!

anxiousmodernman commented Apr 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lambda-fairy commented Apr 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anxiousmodernman commented Apr 12, 2018

Uh oh!

anxiousmodernman commented Apr 12, 2018

Uh oh!

anxiousmodernman commented Apr 16, 2018

Uh oh!

lambda-fairy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

anxiousmodernman commented Apr 9, 2018 •

edited

Loading

anxiousmodernman commented Apr 9, 2018 •

edited

Loading

anxiousmodernman commented Apr 12, 2018 •

edited

Loading

lambda-fairy commented Apr 12, 2018 •

edited

Loading