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

RESOLVED FIXED

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: cpeterson, Assigned: cpeterson)

Tracking

(Blocks 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox57 wontfix, firefox58 wontfix, firefox59 fixed)

Details

Attachments

(14 attachments)

13.20 KB, patch
hsivonen
: review+
Details | Diff | Splinter Review
1.07 KB, patch
hsivonen
: review+
Details | Diff | Splinter Review
2.22 KB, patch
hsivonen
: review+
Details | Diff | Splinter Review
1.57 KB, patch
hsivonen
: review+
Details | Diff | Splinter Review
1.54 KB, patch
hsivonen
: review+
Details | Diff | Splinter Review
3.02 KB, patch
hsivonen
: review+
Details | Diff | Splinter Review
2.46 KB, patch
hsivonen
: review+
Details | Diff | Splinter Review
1.94 KB, patch
hsivonen
: review+
Details | Diff | Splinter Review
480.79 KB, patch
hsivonen
: review+
Details | Diff | Splinter Review
153.64 KB, patch
hsivonen
: review+
Details | Diff | Splinter Review
231.87 KB, patch
hsivonen
: review+
Details | Diff | Splinter Review
6.67 KB, patch
hsivonen
: review+
Details | Diff | Splinter Review
2.79 KB, patch
hsivonen
: review+
Details | Diff | Splinter Review
10.72 KB, patch
hsivonen
: review+
Details | Diff | Splinter Review
Assignee

Description

2 years ago
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.
Assignee

Comment 2

2 years ago
The NS_ABORT_IF_FALSE macro no longer exists.
Attachment #8936092 - Flags: review?(hsivonen)
Assignee

Comment 3

2 years ago
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.
Attachment #8936091 - Flags: review?(hsivonen)
Assignee

Comment 5

2 years ago
#include "jArray.h"
#include "nscore.h"
#include "nsDebug.h"
#include "mozilla/Logging.h"
#include "nsMemory.h"
Attachment #8936094 - Flags: review?(hsivonen)
Assignee

Comment 9

2 years ago
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
Attachment #8936098 - Flags: review?(hsivonen)
Assignee

Comment 10

2 years ago
And remove always-zero offset from CPPONLY code

block bug 500617
Attachment #8936099 - Flags: review?(hsivonen)
Assignee

Comment 12

2 years ago
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.
Attachment #8936101 - Flags: review?(hsivonen)
Assignee

Comment 14

2 years ago
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.
Attachment #8936103 - Flags: review?(hsivonen)
Assignee

Comment 15

2 years ago
Overridden virtual functions without override specifiers will become errors if/when I enable gcc -Wsuggest-override warnings.
Attachment #8936104 - Flags: review?(hsivonen)
Attachment #8936092 - Flags: review?(hsivonen) → review+
Attachment #8936093 - Flags: review?(hsivonen) → review+
Attachment #8936094 - Flags: review?(hsivonen) → review+
Attachment #8936095 - Flags: review?(hsivonen) → review+
Attachment #8936096 - Flags: review?(hsivonen) → review+
Attachment #8936097 - Flags: review?(hsivonen) → review+
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.
Attachment #8936098 - Flags: review?(hsivonen) → review+
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.
Attachment #8936099 - Flags: review?(hsivonen) → review+
Attachment #8936100 - Flags: review?(hsivonen) → review+
Attachment #8936101 - Flags: review?(hsivonen) → review+
Attachment #8936102 - Flags: review?(hsivonen) → review+
Attachment #8936103 - Flags: review?(hsivonen) → review+
Attachment #8936104 - Flags: review?(hsivonen) → review+
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?
Assignee

Comment 19

a year ago
(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 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.
Attachment #8936091 - Flags: review?(hsivonen) → review+

Comment 21

a year ago
Pushed by cpeterson@mozilla.com:
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
Assignee

Comment 22

a year ago
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
Flags: needinfo?(hsivonen)
Keywords: leave-open
(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.
Flags: needinfo?(hsivonen)
Assignee

Updated

a year ago
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.