Closed Bug 1424420 Opened 2 years ago Closed 2 years ago

Specialize functions in Parser<{Full,Syntax}ParseHandler, CharT> in a manner that doesn't require duplicate textual definitions when multiple CharT are possible

Categories

(Core :: JavaScript Engine, enhancement, P2)

58 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: Waldo, Assigned: Waldo)

References

Details

Attachments

(3 files)

Right now we have (with some handwavery here)

template<class ParseHandler, typename CharT>
class Parser
{
   ...
   Node importDeclaration();
   ...
};

template<>
Node
Parser<FullParseHandler, char16_t>::importDeclaration()
{
   ...full definition...
}

template<>
Node
Parser<SyntaxParseHandler, char16_t>::importDeclaration()
{
    return abortIfSyntaxParser();
}

This works to have specialized behavior for full/syntax parsing now.  But when we will also have Parser support for single-byte text, we can't just do, e.g.

template<typename CharT>
Node
Parser<FullParseHandler, CharT>::importDeclaration()
{
   ...full definition...
}

because this is function partial specialization.  And C++ no likey.

The solution I've reached is this:

1) Rename Parser to GeneralParser, and make it not final.
2) Introduce

  template<classParseHandler, typename CharT> class Parser;

  template<typename CharT>
  class Parser<SyntaxParseHandler, CharT> final
    : public GeneralParser<SyntaxParseHandler, CharT>
  {
     ...
     Node importDeclaration();
     ...
  };

  template<typename CharT>
  class Parser<FullParseHandler, CharT> final
    : public GeneralParser<FullParseHandler, CharT>
  { 
     ...
     Node importDeclaration();
     ...
  };

3) Now this *will* work (because class partial specialization *is* permitted, and you're not doing the bad thing to the function):

template<typename CharT>
Node
Parser<FullParseHandler, CharT>::importDeclaration()
{
   ...full definition...
}

template<typename CharT>
Node
Parser<SyntaxParseHandler, CharT>::importDeclaration()
{
   return abortIfSyntaxParser();
}

Of course there's a bit more trickiness than that -- sometimes a function that *remains* in GeneralParser, wants to call one that lives in the Parser specializations, and other things.  But those can be worked around.  The workarounds are:

GeneralParser functions wanting to call Parser functions:
Add a static downcast function to GeneralParser, then add a private inline function in GeneralParser that downcasts and calls accordingly.  So for importDeclaration you might have

template<class ParseHandler, typename CharT>
inline Node
GeneralParser<ParseHandler, CharT>::importDeclaration()
{
    return asFinalParser()->importDeclaration();
}

Parser wanting to use inherited functions/fields/types:
Because GeneralParser<PH, CharT> is a dependent name, every reference to a field/function in it has to be via a dependent name.  The solution everyone understands, is to use |this->| everywhere.  But that's a big mess, so instead this adds |using Base::tokenStream;| to the two specializations, eliminating the need to touch hundreds of lines of code.  The big list of |using|s isn't great, but it's way better than the alternative.

Multiple GP<...>::* function overloads with different access rules:
You can't |using Base::foo;| if |foo| is private.  I forwarded from Parser to GeneralParser in this case using an inline function -- or by renaming one of the underlying overloads.  (Note particularly that Parser::innerFunction is now two differently-named functions, which is way way way less confusing to read, entirely independent of this bug's aim.)

AutoAwaitIsKeyword:
I changed how you invoke AAIK so that it takes GeneralParser instead of Parser, so that users wouldn't have to pass a slightly weird asFinalParser().  That bumps line lengths a bit to squeeze in a GP<...>; it's possible I'll think of a slightly nicer way to skin that cat, but this *does* work now.

There are additional simplifications that can be done atop this -- if you specialize Parser::exportDeclaration, for example, then you can move *all* the different export-handling functions from GeneralParser into Parser<FullParseHandler, CharT> and kill off a bunch of dumb GeneralParser<ParseHandler, CharT>::* export functions that just do |asFinalParser()->exportGoop()|.  But this is a gonzo patch already, and such is way better deferred than piling on.  (I suppose I could have done it before this patch, too -- but I didn't think of it til I was most of the way into this, and the work to flip that is not worth the pain.)
Attached patch PatchSplinter Review
Unfortunately, this patch is more or less not split-uppable.  Fortunately, a great deal of the changing is in whitespace or just adding "General" before "Parser".  I'll post a -w version after this for additional helpfulness.
Attachment #8935940 - Flags: review?(arai.unmht)
Attached patch Patch with -wSplinter Review
This seems like adequate improvement on the AAIK system mentioned in comment 0.
Attachment #8936079 - Flags: review?(arai.unmht)
I'll review them tomorrow.
Comment on attachment 8935940 [details] [diff] [review]
Patch

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

::: js/src/frontend/Parser.h
@@ +1135,5 @@
> +    bool checkExportedNameForFunction(Node node);
> +    bool checkExportedNameForClass(Node node);
> +    inline bool checkExportedNameForClause(Node node);
> +
> +    bool trySyntaxParseInnerFunction(Node pn, HandleFunction fun, uint32_t toStringStart,

both "pn" and "node" are used for genera/syntax/full parsers, in declaration and definition.
it might be nice to align to either.

@@ +1141,5 @@
> +                                     FunctionSyntaxKind kind, GeneratorKind generatorKind,
> +                                     FunctionAsyncKind asyncKind, bool tryAnnexB,
> +                                     Directives inheritedDirectives, Directives* newDirectives);
> +
> +    bool skipLazyInnerFunction(Node node, uint32_t toStringStart, FunctionSyntaxKind kind,

same here (pn vs node).
Attachment #8935940 - Flags: review?(arai.unmht) → review+
Attachment #8936079 - Flags: review?(arai.unmht) → review+
Better than |node|, I went with |funcNode| so as to clarify just what node it is at various use sites (especially when |node| would be forwarded in a big garbage-list argument set and it'd be not clear just what it was).  Made it a separate patch-atop rather than touch the mega-patch here with that mostly cosmetic change.

I expect to land this pretty much Any Day Now, along with several other bugs and changes, as one dozen-rev landing, once I get two last reviews from :Yoric in another bug.
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f31a702d5de
Rename Parser to GeneralParser, and move all partially-specialized (by ParseHandler) functions in GeneralParser into a new Parser class inheriting from GeneralParser.  This permits these functions to be specialized without having two identical definitions when CharT isn't solely char16_t.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/49a259667582
Change the name of some |Node pn| in function-parsing functions to |Node funcNode|.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/bdb40755d36a
Adjust AutoAwaitIsKeyword template parameters slightly so a full parser type need not be written out.  r=arai
https://hg.mozilla.org/mozilla-central/rev/9f31a702d5de
https://hg.mozilla.org/mozilla-central/rev/49a259667582
https://hg.mozilla.org/mozilla-central/rev/bdb40755d36a
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.