Sylvestre Ledru [:Sylvestre] Assignee
	Description • 7 years ago

In file included from /root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/parser/html/Unified_cpp_parser_html1.cpp:92:
 /root/firefox-gcc-last/parser/html/nsHtml5TreeBuilder.cpp: In member function 'void nsHtml5TreeBuilder::characters(const char16_t*, int32_t, int32_t)':
 /root/firefox-gcc-last/parser/html/nsHtml5TreeBuilder.cpp:349:13: error: this statement may fall through [-Werror=implicit-fallthrough=]
              switch (mode) {
              ^~~~~~
 /root/firefox-gcc-last/parser/html/nsHtml5TreeBuilder.cpp:403:11: note: here
            default: {
            ^~~~~~~
 cc1plus: all warnings being treated as errors

Building with --enable-warnings-as-error

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11)
	Comment 3 • 7 years ago

On the mechanics of landing:
This change needs to be made in the Java source, which is out of sync due to the Java part of bug 1424548 not landing due to bug 1427696. That needs to be sorted out before further changes.

As for the change itself:
I'll have to do another review pass, but on surface, it looks to me like this shouldn't fall through but what looks to the compiler like a fall-through should be some kind of "not reached" assertion. But I'll take another look before claiming this for certain.

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11)
	Comment 4 • 7 years ago

Increasingly convinced that this is not reached rather than a fall-through. Still investigating...

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11)
	Comment 5 • 7 years ago

None of the html5lib test cases exercise this line, which increases confidence that it's not reached. Still investigating...

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11)
	Comment 6 • 7 years ago

Comment on attachment 8939849 [details]
Bug 1426997 - Add MOZ_FALLTHROUGH_ASSERT to make it clear that it can never fall through

Please use a "not reached" annotation instead of a "fall through" annotation. This can never fall through. The switch is exhaustive except for TEXT, but TEXT is handled with an early return in the outer switch.

	Chris Peterson [:cpeterson]
	Comment 8 • 7 years ago

While MOZ_ASSERT_UNREACHABLE may suppress the fallthrough warning in debug builds, MOZ_ASSERT_UNREACHABLE is a no-op in opt builds and gcc will complain again. There is a MOZ_FALLTHROUGH_ASSERT macro if you want to assert in debug builds but fallthrough without warning in opt builds.

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11)
	Comment 10 • 7 years ago
mozreview-review

Comment on attachment 8939849 [details]
Bug 1426997 - Add MOZ_FALLTHROUGH_ASSERT to make it clear that it can never fall through

https://reviewboard.mozilla.org/r/209322/#review223242

Looks good. Needs to be synced with the Java sources, though, so that it doesn't get overwritten.

	Chris Peterson [:cpeterson]
	Comment 11 • 7 years ago

Sylvestre, I recently updated the HTML5 parser's Java sources (for bug 1424548), so I already have the Java sources repo on my computer and am familiar with the process. I can easily submit a follow-up Java patch, if you like.

The Java changes will look something like this:

https://hg.mozilla.org/mozilla-central/diff/7ea2b1f67e08/parser/html/javasrc/MetaScanner.java

	Chris Peterson [:cpeterson]
	Comment 12 • 7 years ago

and the corresponding changes in the upstream Java sources repo:

https://hg.mozilla.org/projects/htmlparser/rev/93bff4c75f39

	Pulsebot
	Comment 13 • 7 years ago

Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/59f5d0e5b7c8
Add MOZ_FALLTHROUGH_ASSERT to make it clear that it can never fall through r=hsivonen

	Cosmin Sabou [:CosminS]
	Comment 14 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/59f5d0e5b7c8