nsHtml5TreeBuilder: Remove unnecessary switch fallthrough to avoid -Wimplicit-fallthrough warning

RESOLVED FIXED in Firefox 60

Status

()

defect
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: sylvestre, Assigned: sylvestre)

Tracking

(Blocks 1 bug)

Trunk
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 wontfix, firefox59 wontfix, firefox60 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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
Posted patch aze.diff (obsolete) — Splinter Review
Attachment #8938795 - Flags: review?(hsivonen)
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.
Depends on: 1427696
Priority: -- → P3
Attachment #8938795 - Attachment is obsolete: true
Attachment #8938795 - Flags: review?(hsivonen)
Increasingly convinced that this is not reached rather than a fall-through. Still investigating...
None of the html5lib test cases exercise this line, which increases confidence that it's not reached. Still investigating...
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.
Attachment #8939849 - Flags: review?(hsivonen) → review-
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.
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.
Attachment #8939849 - Flags: review?(hsivonen) → review+
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
and the corresponding changes in the upstream Java sources repo:

https://hg.mozilla.org/projects/htmlparser/rev/93bff4c75f39
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
https://hg.mozilla.org/mozilla-central/rev/59f5d0e5b7c8
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.