1424548 - Regenerate HTML parser code to redo formatting and add MOZ_FALLTHROUGH and override annotations

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, enhancement, P3)

Description

[Chris Peterson [:cpeterson]]

Bug 1400460 renamed nsIAtom to nsAtom in the generated parser code (and reformatted it) without updating the Java translator.

Bug 500617 removed always-zero offsets in the generated parser code, but missed one in CPPONLY code in a parser Java file.

Bug 1236322 suppressed clang -Wimplicit-fallthrough warnings in the generated parser code because it did not include MOZ_FALLTHROUGH annotations.

Comment 1

[Chris Peterson [:cpeterson]]

Attachment: Part 1: Fix non-unified build of HTML parser code

Comment 2

[Chris Peterson [:cpeterson]]

Attachment: Part 2: Remove unused abortIfFalse method

The NS_ABORT_IF_FALSE macro no longer exists.

Comment 3

[Chris Peterson [:cpeterson]]

Comment on attachment 8936091 [details] [diff] [review]
Part 1: Fix non-unified build of HTML parser code

Part 1: Fix non-unified build of HTML parser code.

Comment 4

[Chris Peterson [:cpeterson]]

Attachment: Part 3: Fix typo in "destructorBoilderplate" method name

Comment 5

[Chris Peterson [:cpeterson]]

Attachment: Part 4: Include the same header files as the current generated parser code

#include "jArray.h"
#include "nscore.h"
#include "nsDebug.h"
#include "mozilla/Logging.h"
#include "nsMemory.h"

Comment 6

[Chris Peterson [:cpeterson]]

Attachment: Part 5: Remove always-zero offset from CPPONLY code to match current generated parser code

block bug 500617

Comment 7

[Chris Peterson [:cpeterson]]

Attachment: Part 6: Emit nsAtom instead of nsIAtom

block bug 1400460

Comment 8

[Chris Peterson [:cpeterson]]

Attachment: Part 7: Emit static_assert instead of PR_STATIC_ASSERT

Comment 9

[Chris Peterson [:cpeterson]]

Attachment: Part 8: Reformat C++ constructor initializer lists

This format matches the modified generated HTML parser code and Mozilla's current C++ style guidelines.

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

Comment 10

[Chris Peterson [:cpeterson]]

Attachment: Part 9: Regenerate HTML parser code to undo formatting changes to made the generated code directly

And remove always-zero offset from CPPONLY code

block bug 500617

Comment 11

[Chris Peterson [:cpeterson]]

Attachment: Part 10: Emit MOZ_FALLTHROUGH instead of "fall through" comments

block bug 1236322

Comment 12

[Chris Peterson [:cpeterson]]

Attachment: Part 11: Regenerate HTML parser code with MOZ_FALLTHROUGH annotations

And stop suppressing -Wimplicit-fallthrough warnings. We no longer need to suppress these clang warnings because the generated parser code now includes MOZ_FALLTHROUGH annotations.

Comment 13

[Chris Peterson [:cpeterson]]

Attachment: Part 12: Add @Override annotations to subclasses implementing the TreeBuilderState interface

Comment 14

[Chris Peterson [:cpeterson]]

Attachment: Part 13: Emit override specifiers in the generated HTML parser code

We convert Java @Override annotations to C++ override specifiers in generated HTML parser code.

Overridden virtual functions without override specifiers will become errors if/when I enable gcc -Wsuggest-override warnings.

Comment 15

[Chris Peterson [:cpeterson]]

Attachment: Part 14: Regenerate HTML parser code with override specifiers

Overridden virtual functions without override specifiers will become errors if/when I enable gcc -Wsuggest-override warnings.

Comment 16

[Henri Sivonen (:hsivonen) (temporarily away from Bugzilla)]

Comment on attachment 8936098 [details] [diff] [review]
Part 8: Reformat C++ constructor initializer lists

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

Running clang-format afterwards overwrites anyway.

Comment 17

[Henri Sivonen (:hsivonen) (temporarily away from Bugzilla)]

Comment on attachment 8936099 [details] [diff] [review]
Part 9: Regenerate HTML parser code to undo formatting changes to made the generated code directly

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

r+ provided that you run ./mach clang-format afterwards.

Comment 18

[Henri Sivonen (:hsivonen) (temporarily away from Bugzilla)]

Comment on attachment 8936091 [details] [diff] [review]
Part 1: Fix non-unified build of HTML parser code

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

::: parser/html/moz.build
@@ +53,5 @@
>      'nsIContentHandle.h',
>      'nsParserUtils.h',
>  ]
>  
> +SOURCES += [

What's the reason not to keep these unified?

Comment 19

[Chris Peterson [:cpeterson]]

(In reply to Henri Sivonen (:hsivonen) from comment #17)
> Part 9: Regenerate HTML parser code to undo formatting changes to made the
> generated code directly
...
> r+ provided that you run ./mach clang-format afterwards.

OK. I'll run ./mach clang-format before checking in.


> > +SOURCES += [
> 
> What's the reason not to keep these unified?

Oops! That's a bug. I meant to change UNIFIED_SOURCES to SOURCES only temporarily to find the non-unified build problems.

Comment 20

[Henri Sivonen (:hsivonen) (temporarily away from Bugzilla)]

Comment on attachment 8936091 [details] [diff] [review]
Part 1: Fix non-unified build of HTML parser code

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

(In reply to Chris Peterson [:cpeterson] from comment #19)
> Oops! That's a bug. I meant to change UNIFIED_SOURCES to SOURCES only
> temporarily to find the non-unified build problems.

I see. r+ provided that you change it back to UNIFIED_SOURCES.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7516d49fc731
Part 1: Fix non-unified build of HTML parser code. r=hsivonen
https://hg.mozilla.org/integration/mozilla-inbound/rev/9874df8448d8
Part 9a: clang-format generated HTML parser code. r=hsivonen
https://hg.mozilla.org/integration/mozilla-inbound/rev/091d01097937
Part 9b: Regenerate HTML parser code to remove always-zero offset from CPPONLY code. r=hsivonen
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ea2b1f67e08
Part 11: Regenerate HTML parser code with MOZ_FALLTHROUGH annotations. r=hsivonen
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fcbcdfba8e5
Part 14: Regenerate HTML parser code with override specifiers. r=hsivonen

Comment 22

[Chris Peterson [:cpeterson]]

Henri, does pushing to the https://hg.mozilla.org/projects/htmlparser repo require different permissions than pushing to inbound? I have Commit Access Level 3 (Core Product Access). I can successfully `hg outgoing` with the htmlparser repo, but `hg push` fails with "abort: authorization failed":

$ hg push --verbose

pushing to https://hg.mozilla.org/projects/htmlparser
searching for changes
10 changesets found
uncompressed size of bundle content:
    5433 (changelog)
    2868 (manifests)
    1026  src/nu/validator/htmlparser/impl/MetaScanner.java
     527  src/nu/validator/htmlparser/impl/StateSnapshot.java
    8655  src/nu/validator/htmlparser/impl/Tokenizer.java
    2291  src/nu/validator/htmlparser/impl/TreeBuilder.java
     288  translator-src/nu/validator/htmlparser/cpptranslate/AnnotationHelperVisitor.java
    1126  translator-src/nu/validator/htmlparser/cpptranslate/CppTypes.java
     827  translator-src/nu/validator/htmlparser/cpptranslate/CppVisitor.java
     326  translator-src/nu/validator/htmlparser/generator/GenerateNamedCharactersCpp.java
abort: authorization failed

Comment 23

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/7516d49fc731
https://hg.mozilla.org/mozilla-central/rev/9874df8448d8
https://hg.mozilla.org/mozilla-central/rev/091d01097937
https://hg.mozilla.org/mozilla-central/rev/7ea2b1f67e08
https://hg.mozilla.org/mozilla-central/rev/1fcbcdfba8e5

Comment 24

[Henri Sivonen (:hsivonen) (temporarily away from Bugzilla)]

(In reply to Chris Peterson [:cpeterson] from comment #22)
> Henri, does pushing to the https://hg.mozilla.org/projects/htmlparser repo
> require different permissions than pushing to inbound?

I thought it didn't, but since the server doesn't allow you or Jessica to push, apparently there's something I don't know. Filed bug 1427696.

Comment 25

[Chris Peterson [:cpeterson]]

Resolving fixed because I finally landed the Java htmlparser translator changes:

https://hg.mozilla.org/projects/htmlparser/rev/95d2e02251df
https://hg.mozilla.org/projects/htmlparser/rev/afa68d0380ed
https://hg.mozilla.org/projects/htmlparser/rev/79a997f5156f
https://hg.mozilla.org/projects/htmlparser/rev/93bff4c75f39
https://hg.mozilla.org/projects/htmlparser/rev/83ed0725ed47
https://hg.mozilla.org/projects/htmlparser/rev/63de2bd615b5
https://hg.mozilla.org/projects/htmlparser/rev/219397dfab48
https://hg.mozilla.org/projects/htmlparser/rev/62abdd11d044
https://hg.mozilla.org/projects/htmlparser/rev/cfd1e575d398
https://hg.mozilla.org/projects/htmlparser/rev/f61ddeeb235d
https://hg.mozilla.org/projects/htmlparser/rev/1e8c5d1b6096