1371105 - Reduce amount of templates in frontend

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Shu-yu Guo [:shu]]

Waldo, in bug 1351107, has added a stupendous amount of templates and base classes to the frontend. I believe many of them are unnecessary, and I'm going to try to get the complexity down for my own sanity.

Comment 1

[Shu-yu Guo [:shu]]

Attachment: De-templatize parse handlers.

The ParseHandlers were made to take a CharT because FullParseHandler
held a pointer to a syntax parser for syntax parsing. This patch moves
that pointer into Parser to detemplatize the handlers. It also collapses
the |abortedSyntaxParse| flag, which was already on the Parser, and
re-uses the new |syntaxParser| field in Parser to track if a syntax
parse was aborted.

Comment 2

[Jason Orendorff [:jorendorff]]

Comment on attachment 8875529 [details] [diff] [review]
De-templatize parse handlers.

Review of attachment 8875529 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing review. After reading these comments, if you still want to proceed with this patch as-is, please re-r?me and ping me on IRC, and I'll review.

::: js/src/frontend/Parser.h
@@ +973,5 @@
>  {
>      useAsVarScope(parser->pc);
>  }
>  
> +template <class ParseHandler, typename CharT>

I have two counter-proposals.

First counter-proposal. I wonder why you couldn't change the entire Parser template back to an even simpler "kind":

    template <class ParseHandler>
    class Parser ...
    {
      ...
      typedef ParseHandler::Char Char;
      ...
    };

Sure, this would leave the handlers still "infected" with Char, which seems a little backwards. But if it works and is sufficient to restore your sanity, this route requires no casting and no fake "sentinel value" pointers.

@@ +1115,5 @@
> +    // When ParseHandler is SyntaxParseHandler:
> +    //
> +    //   If non-null, this field must be a sentinel value signaling that the
> +    //   syntax parse was aborted. If null, then lazy parsing was aborted due
> +    //   to encountering unsupported language constructs.

This is a lot to take in. Maybe there's a cleaner hack here.

Second counter-proposal. Instead of moving the syntaxParser field to the Parser:

*   FullParseHandler::syntaxParser stays, but it becomes a ParserBase* and we
    static_cast it to the right type in the parser when needed (a little gross, but
    so is the "sentinel value"; and this is the minimal hack for the exact goal
    you're trying to achieve)

*   SyntaxParseHandler gets a `bool disabled;`

*   they both keep their disableSyntaxParser() methods, and add others as needed
    so that handler-specific code stays in the handler classes when possible

----

You always comment the things that most need it. It's so convenient. I could review your patches just by searching for the longest comment added... :)

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 8875529 [details] [diff] [review]
De-templatize parse handlers.

shu says on irc "I think #2 and mine are about equal savoriness in my mind" so I'll review the patch as-is.

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 8875529 [details] [diff] [review]
De-templatize parse handlers.

Review of attachment 8875529 [details] [diff] [review]:
-----------------------------------------------------------------

Ultimately, I guess I agree that a single pointer field is not a good enough reason to HKT up the whole Parser. r=me. But it's a closer call than I thought at first.

::: js/src/frontend/Parser.cpp
@@ +768,5 @@
> +#define ABORTED_SYNTAX_PARSE_SENTINEL (SyntaxParser*)(0x1)
> +
> +template <>
> +bool
> +Parser<FullParseHandler, char16_t>::hadAbortedSyntaxParse()

template <typename CharT>
bool
Parser<FullParseHandler, CharT>::...

Right?

@@ +797,5 @@
> +template <>
> +void
> +Parser<FullParseHandler, char16_t>::disableSyntaxParser()
> +{
> +    syntaxParser_ = nullptr;

If you wanted to flip the sense of this so that null always means disabled, I wouldn't object, but it looks inconvenient actually doing it... Bleargh.

::: js/src/frontend/Parser.h
@@ +1187,1 @@
>      inline bool abortIfSyntaxParser();

Thank you for this comment. In the latter case, please specify the "scope" of disabling syntax parsing ("within the current function" or "throughout the current compilation unit").

@@ +1188,5 @@
>  
> +    // If ParseHandler is SyntaxParseHandler, do nothing.
> +    //
> +    // If ParseHandler is FullParseHandler, disable syntax parsing of inner
> +    // functions.

Or maybe here

Comment 5

[Shu-yu Guo [:shu]]

(In reply to Jason Orendorff [:jorendorff] from comment #4)
> Comment on attachment 8875529 [details] [diff] [review]
> De-templatize parse handlers.
> 
> Review of attachment 8875529 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ultimately, I guess I agree that a single pointer field is not a good enough
> reason to HKT up the whole Parser. r=me. But it's a closer call than I
> thought at first.
> 
> ::: js/src/frontend/Parser.cpp
> @@ +768,5 @@
> > +#define ABORTED_SYNTAX_PARSE_SENTINEL (SyntaxParser*)(0x1)
> > +
> > +template <>
> > +bool
> > +Parser<FullParseHandler, char16_t>::hadAbortedSyntaxParse()
> 
> template <typename CharT>
> bool
> Parser<FullParseHandler, CharT>::...
> 
> Right?

Specializations are currently hardcoding char16_t until single-byte lexing is actually implemented. Easier to keep consistent for search and replace.

> 
> @@ +797,5 @@
> > +template <>
> > +void
> > +Parser<FullParseHandler, char16_t>::disableSyntaxParser()
> > +{
> > +    syntaxParser_ = nullptr;
> 
> If you wanted to flip the sense of this so that null always means disabled,
> I wouldn't object, but it looks inconvenient actually doing it... Bleargh.
> 

I tried this, it was really inconvenient when you need to clear aborted syntax parses. Also, "disabled" is really a different concept than "aborted". Calling disableSyntaxParser() on a SyntaxParser is a no-op, not an abort.

> ::: js/src/frontend/Parser.h
> @@ +1187,1 @@
> >      inline bool abortIfSyntaxParser();
> 
> Thank you for this comment. In the latter case, please specify the "scope"
> of disabling syntax parsing ("within the current function" or "throughout
> the current compilation unit").
> 
> @@ +1188,5 @@
> >  
> > +    // If ParseHandler is SyntaxParseHandler, do nothing.
> > +    //
> > +    // If ParseHandler is FullParseHandler, disable syntax parsing of inner
> > +    // functions.
> 
> Or maybe here

Amended the comments to:

    // If ParseHandler is SyntaxParseHandler, flag the current syntax parse as
    // aborted due to unsupported language constructs and return
    // false. Aborting the current syntax parse does not disable attempting to
    // syntax parse future inner functions.
    //
    // If ParseHandler is FullParseHandler, disable syntax parsing of all
    // future inner functions and return true.
    inline bool abortIfSyntaxParser();

    // If ParseHandler is SyntaxParseHandler, do nothing.
    //
    // If ParseHandler is FullParseHandler, disable syntax parsing of all
    // future inner functions.
    void disableSyntaxParser();

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/07fcbdb153e2
De-templatize parse handlers. (r=jorendorff)

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/07fcbdb153e2