1508063 - BinSource-auto.h needs to be compatible with clang-format

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P1)

Description

[Ted Campbell [:tcampbell]]

This file is auto-generated by currently scheduled to be updated by clang-format. It might make sense to add this to ignore file, but the generator should also be eventually changed so that the generated result was clang-format compatible.

Comment 1

[Ted Campbell [:tcampbell]]

Yoric, can you track this issue?

Comment 2

[Tooru Fujisawa [:arai]]

I'll take

Comment 3

[Tooru Fujisawa [:arai]]

given we can use clang-format, the code generator shouldn't think about formatting I think.
and we should make the code-generation simpler as much as possible (like, remove newline/space handling)

of course the C++ codelet there should follow the rule tho

Comment 4

[Tooru Fujisawa [:arai]]

then, about header, I think the simplest solution is to separate the class for auto-generated part and others

maybe
  BinASTParserBase
    ^
    |  ErrorReporter
    |    ^
    |    |  BCEParserHandle
    |    |    ^
    |    |    |
  BinASTParserGenerated
    ^
    |
  BinASTParser

so that the auto-generated part doesn't have to be included in other source.

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Move BinASTParserBase into its own files.

The entire plan is a bit changed from the above comments (already marked obsolete).

The class hierarchy becomes the following (due to the dependency between methods), where:
  * BinASTParser (in BinASTParser.{cpp,h})
      contains auto-generated methods, which was formerly in BinSource-auto.{cpp,h}
  * BinASTParserPerTokenizer (in BinASTParserPerTokenizer.{cpp,h})
      contains non-auto-generated methods, which was formerly in BinSource.{cpp,h}

maybe
  BinASTParserBase
    ^
    |  ErrorReporter
    |    ^
    |    |  BCEParserHandle
    |    |    ^
    |    |    |
  BinASTParserPerTokenizer
    ^
    |
  BinASTParser

This change consist of 5 patches.

As part of the above change, I've renamed some source from BinSource to binast/BinASTParser,
the remaining will be done in bug 1505343.

try is running:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbc35caad5c35afbddaaa17b3d9400da87174f7b


[Part 1]

Maybe unnecessary, but just for simplicity.
Moved BinASTParserBase to its own file BinASTParserBase.{cpp,h}.

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Move auto-generated enum to BinASTEnum.h.

[Part 2]

Moved enum to BinASTEnum.h with "binast" namespace,
to make it easier to use it from both BinASTParserPerTokenizer.h and BinASTParser.h (added in Part 4)

changes:
  * Comment for each enum item is now in its own line,
    given spaces will be removed by clang-format and the alignment will be broken
  * Added hpp.enums.{header,footer} fields to BinSource.yaml

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Fix typo in Binsource.yaml.

[Part 3]

not relatd, but while working on this, noticed the field name is wrong in BinSource.yaml.
fixed it.

Comment 8

[Tooru Fujisawa [:arai]]

Attachment: Part 4: Rename BinSource-auto.{cpp,h} to BinASTParser.{cpp,h}, and BinSource.{cpp,h} to BinASTParserPerTokenizer.{cpp,h}.

[Part 4]

Just for ease of review, separated file-rename and others (which is in Part 5),
so that the diff is smaller in non-simple part
  * Rename BinSource-auto.{cpp,h} to BinASTParser.{cpp,h}
  * Rename BinSource.{cpp,h} to BinASTParserPerTokenizer.{cpp,h}

The content is not much changed (just #include filename and order)

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: Part 5: Move non-auto-generated part of BinASTParser into BinASTParserPerTokenizer.

[Part 5]

Consists of the following changes:
  * Rename formerly non-auto-generated part of BinASTParser to BinASTParserPerTokenizer
  * The formerly auto-generated part remains BinASTParser
  * BinASTParser inherits BinASTParserPerTokenizer
  * Make BinASTParser.h (formerly BinSource-auto.h) a standalone header,
    instead of to-be-#include-d one.
  * Added hpp.class.footer field to BinSource.yaml, to generate the closing part of class
  * Fixed the indentation for items in BinASTParser.h (formerly BinSource-auto.h)
  * 2 parts in BinASTParserPerTokenizer requires accessing methods in BinASTParser,
    so added `BinASTParserPerTokenizer::asFinalParser()` which returns `BinASTParser*`.
    (this is same as regular parser's situation)

some note:
  * BinASTParser.h contains a bunch of "using", this is necessary for the following reasons:
    * we use template
    * base class's things are not accessible without `this->` because of above
    * we don't use `this->`, at least in most case
      (I hope we switch the rule...)
  * In some case we still need `this->` and/or `template` keyword
    (like `this->template new_<...>(...)`)

Comment 10

[Tooru Fujisawa [:arai]]

about bug 1508062 comment #1, I'm not going to uplift these patches to esr60, given we don't use binast on non-nightly branch.

Comment 11

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 9028172 [details] [diff] [review]
Part 3: Fix typo in Binsource.yaml.

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

Ah, good catch :)

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 9028174 [details] [diff] [review]
Part 5: Move non-auto-generated part of BinASTParser into BinASTParserPerTokenizer.

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

::: js/src/frontend/BinASTParserPerTokenizer.cpp
@@ +797,5 @@
> +inline typename BinASTParserPerTokenizer<Tok>::FinalParser*
> +BinASTParserPerTokenizer<Tok>::asFinalParser()
> +{
> +    static_assert(mozilla::IsBaseOf<BinASTParserPerTokenizer<Tok>, FinalParser>::value,
> +                  "inheritance relationship required by the static_cast<> below");

I don't understand this error message.

::: js/src/frontend/BinASTParserPerTokenizer.h
@@ +62,4 @@
>      using VariableDeclarationKind = binast::VariableDeclarationKind;
>  
> +  private:
> +    using FinalParser = BinASTParser<Tok>;

Could you document what `FinalParser` is?

@@ +299,4 @@
>          MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER
>      };
> +
> +    inline FinalParser* asFinalParser();

These two methods, too, deserve some documentation.

::: js/src/frontend/BinSource.yaml
@@ +82,5 @@
>  
>              // To generate this file, see the documentation in
>              // js/src/frontend/binsource/README.md.
>  
> +            #ifndef frontend_BinASTParser_h

That's lots of code that suddenly becomes harder to edit and review. Was there no better way?

@@ +842,5 @@
>      inherits: FunctionExpressionContents
>      fields:
>          body:
>              before: |
> +                BINJS_TRY_DECL(params, this->template new_<ListNode>(ParseNodeKind::ParamsBody, tokenizer_->pos(start)));

Mmmhhh... What does the keyword `template` mean in this context?

Comment 13

[Tooru Fujisawa [:arai]]

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #12)
> ::: js/src/frontend/BinASTParserPerTokenizer.cpp
> @@ +797,5 @@
> > +inline typename BinASTParserPerTokenizer<Tok>::FinalParser*
> > +BinASTParserPerTokenizer<Tok>::asFinalParser()
> > +{
> > +    static_assert(mozilla::IsBaseOf<BinASTParserPerTokenizer<Tok>, FinalParser>::value,
> > +                  "inheritance relationship required by the static_cast<> below");
> 
> I don't understand this error message.

it's just a copy of the following:
https://searchfox.org/mozilla-central/rev/0859e6b10fb901875c80de8f8fc33cbb77b2505e/js/src/frontend/Parser.cpp#112-115

GeneralParser/Parser has similar issue and uses this way.


> ::: js/src/frontend/BinSource.yaml
> @@ +82,5 @@
> >  
> >              // To generate this file, see the documentation in
> >              // js/src/frontend/binsource/README.md.
> >  
> > +            #ifndef frontend_BinASTParser_h
> 
> That's lots of code that suddenly becomes harder to edit and review. Was
> there no better way?

As a quick fix, this would be simplest I think.
we could auto-generate "using" part, but I'd like to leave it to other bug,
given the deadline is reaching.


> @@ +842,5 @@
> >      inherits: FunctionExpressionContents
> >      fields:
> >          body:
> >              before: |
> > +                BINJS_TRY_DECL(params, this->template new_<ListNode>(ParseNodeKind::ParamsBody, tokenizer_->pos(start)));
> 
> Mmmhhh... What does the keyword `template` mean in this context?

It's there to make "<" and ">" a template parameter syntax, instead of comparison operator.

because of inheritance with template classes and template method,
"new_" there itself is not clear that it's template method of base class,
and those "this->" and "template" are necessary for successful compilation.

Comment 14

[Tooru Fujisawa [:arai]]

Attachment: Part 5: Move non-auto-generated part of BinASTParser into BinASTParserPerTokenizer.

Added comment for asFinalParser.

To clarify, my understanding of this bug is:
  1. BinSource-auto.h is not top-level code
  2. code generator generates code with SpiderMonkey coding style

After this patch, the remaining issues in the code generator are:
  1. hpp.class.header is long
     we could reduce the "using"s, but it may take long (using for base class things needs parsing C++ code)
  2. integrate clang-format (bug 1510586)
     this is not immediately necessary, we just need to manually run it after build.sh

Comment 15

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2abc868cd4169ab888b5a5e7d7ebd5687b06232d
Bug 1508063 - Part 1: Move BinASTParserBase into its own files. r=Yoric

https://hg.mozilla.org/integration/mozilla-inbound/rev/391d7669e9b28c4475ead6f5f6771dfa1fd05deb
Bug 1508063 - Part 2: Move auto-generated enum to BinASTEnum.h. r=Yoric

https://hg.mozilla.org/integration/mozilla-inbound/rev/f96f280927ff0bc9340a7726d28851f884998126
Bug 1508063 - Part 3: Fix typo in Binsource.yaml. r=Yoric

https://hg.mozilla.org/integration/mozilla-inbound/rev/02386132c1e9bfa3ef88b9ae7d967f90b6e1045e
Bug 1508063 - Part 4: Rename BinSource-auto.{cpp,h} to BinASTParser.{cpp,h}, and BinSource.{cpp,h} to BinASTParserPerTokenizer.{cpp,h}. r=Yoric

https://hg.mozilla.org/integration/mozilla-inbound/rev/44cd6ec4fbd5da73777883f4faf9fe822f7de513
Bug 1508063 - Part 5: Move non-auto-generated part of BinASTParser into BinASTParserPerTokenizer. r=Yoric

Comment 16

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/2abc868cd416
https://hg.mozilla.org/mozilla-central/rev/391d7669e9b2
https://hg.mozilla.org/mozilla-central/rev/f96f280927ff
https://hg.mozilla.org/mozilla-central/rev/02386132c1e9
https://hg.mozilla.org/mozilla-central/rev/44cd6ec4fbd5