1603919 - Replace MOZ_FALLTHROUGH macro with C++17's [[fallthrough]] attribute in htmlparser code generator

Status: RESOLVED FIXED

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

Description

[Chris Peterson [:cpeterson]]

Bug 1570499 is replacing Mozilla's MOZ_FALLTHROUGH macro with C++17's [[fallthrough]] attribute. Some of these MOZ_FALLTHROUGH uses are in generated htmlparser code. This bug updates the htmlparser's code generator to emit [[fallthrough]] instead of MOZ_FALLTHROUGH.

Unfortunately, the MOZ_FALLTHROUGH_ASSERT macro (to assert on case fallthrough in debug builds) is still necessary after switching from MOZ_FALLTHROUGH (i.e. [[clang::fallthrough]]) to [[fallthrough]] because:

• MOZ_ASSERT(false) followed by [[fallthrough]] triggers a -Wunreachable-code warning in DEBUG builds
• but MOZ_ASSERT(false) without [[fallthrough]] triggers a -Wimplicit-fallthrough warning in NDEBUG builds.

Comment 1

[Chris Peterson [:cpeterson]]

Bug 1600545 removed useless #includes from many .cpp files, including some of htmlparser's generated .cpp files without updating the htmlparser code generator:

https://hg.mozilla.org/mozilla-central/rev/1f438b01c7801e7d750c4297330669621d3b7a4f

Bug 1446097 reformatted some headers and whitespace in htmlparser's generated code without updating the htmlparser code generator:

https://hg.mozilla.org/mozilla-central/rev/6af1f5ac596b4aa0a87f8a8151395c692eb81a58

Comment 2

[Chris Peterson [:cpeterson]]

Attachment: htmlparser-replace-MOZ_FALLTHROUGH-with-fallthrough.patch

Henri, I'm not sure how to submit changes for https://hg.mozilla.org/projects/htmlparser/ via Phabricator. Here is a patch file. If you'd like me to submit the change some other way, just let me know.

Mozilla bug 1603919 - Replace MOZ_FALLTHROUGH macro with C++17's [[fallthrough]] attribute in htmlparser code generator r?hsivonen

Bug 1570499 is replacing Mozilla's MOZ_FALLTHROUGH macro with C++17's [[fallthrough]] attribute. Some of these MOZ_FALLTHROUGH uses are in generated htmlparser code. This changeset updates the htmlparser's code generator to emit [[fallthrough]] instead of MOZ_FALLTHROUGH.

Comment 3

[Chris Peterson [:cpeterson]]

Attachment: Bug 1603919 - Part 1: Restore htmlparser's #include "nsITimer.h" after `make sync`. r?hsivonen

Bug 1600545 removed useless #includes from many .cpp files, including some of htmlparser's generated .cpp files [1] without updating the htmlparser code generator. This changeset restores these #includes so mozilla-central's htmlparser code matches the output of make sync.

Ideally, the htmlparser code generator would be updated to not emit these #includes. However, I don't know where these includes are coming from because I don't see the substring nsITimer.h in the code generator's Java code.

[1] https://hg.mozilla.org/mozilla-central/rev/1f438b01c7801e7d750c4297330669621d3b7a4f

Comment 4

[Chris Peterson [:cpeterson]]

Attachment: Bug 1603919 - Part 2: Restore htmlparser's #includes and whitespace after `make named_characters`. r?hsivonen

Bug 1446097 reformatted some headers and whitespace in htmlparser's generated code [1] without updating the htmlparser code generator. This changeset restores these #includes and whitespace so mozilla-central's htmlparser code matches the output of make named_characters.

Ideally, the htmlparser code generator would be updated to emit those #includes and whitespace.

[1] https://hg.mozilla.org/mozilla-central/rev/6af1f5ac596b4aa0a87f8a8151395c692eb81a58

Depends on D57231

Comment 5

[Henri Sivonen (:hsivonen)]

Comment on attachment 9115977 [details] [diff] [review]
htmlparser-replace-MOZ_FALLTHROUGH-with-fallthrough.patch

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

This probably won't work, because `[[fallthrough]] is a syntax error in Java 5. This needs the detector to special-case the symbol `MOZ_FALLTHROUGH` to output `[[fallthrough]]`.

Comment 6

[Henri Sivonen (:hsivonen)]

Attachment: Translator patch

(Using feedback? for r?, since r? is unavailable.)

Comment 7

[Henri Sivonen (:hsivonen)]

Attachment: Bug 1603919 - Use [[fallthrough]] instead of MOZ_FALLTHROUGH in generated HTML parser code.

Comment 8

[Henri Sivonen (:hsivonen)]

Let's deal with "part 2" in bug 1604456.

Comment 9

[Chris Peterson [:cpeterson]]

(In reply to Henri Sivonen (:hsivonen) from comment #5)

This probably won't work, because [[fallthrough]] is a syntax error in Java 5. This needs the detector to special-case the symbolMOZ_FALLTHROUGHto output[[fallthrough]]`.

[[fallthrough]] will be a syntax error for the Java parser even though it's still wrapped in a // CPPONLY: [[fallthrough]]; comment?

Comment 10

[Chris Peterson [:cpeterson]]

Comment on attachment 9116363 [details] [diff] [review]
Translator patch

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

LGTM

::: translator-src/nu/validator/htmlparser/cpptranslate/CppTypes.java
@@ +81,4 @@
>          reservedWords.add("unicode");
>      }
>  
> +	private static final String[] TREE_BUILDER_INCLUDES = { "nsContentUtils", "nsAtom", "nsHtml5AtomTable",

I assume you verified that the generated htmlparser still compiles with mozilla-central?

I see there are references to nsITimer.h header and code in parser/html/nsHtml5StreamParser.h and .cpp. They're not generated files, but perhaps there is some accidental header dependency with the generated nsHtml5TreeBuilder files.

Comment 11

[Henri Sivonen (:hsivonen)]

(In reply to Chris Peterson [:cpeterson] from comment #10)

Comment on attachment 9116363 [details] [diff] [review]
Translator patch

Review of attachment 9116363 [details] [diff] [review]:

LGTM

::: translator-src/nu/validator/htmlparser/cpptranslate/CppTypes.java
@@ +81,4 @@

     reservedWords.add("unicode");
 }

• private static final String[] TREE_BUILDER_INCLUDES = { "nsContentUtils", "nsAtom", "nsHtml5AtomTable",

I assume you verified that the generated htmlparser still compiles with
mozilla-central?

I did.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0a5c779695ad
Use [[fallthrough]] instead of MOZ_FALLTHROUGH in generated HTML parser code. r=cpeterson

Comment 13

[Henri Sivonen (:hsivonen)]

Thanks.

https://hg.mozilla.org/projects/htmlparser/rev/4386b9a35eacb91192a99381f2d68f90bdc5f587

Comment 14

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/0a5c779695ad