Closed Bug 1447480 Opened 2 years ago Closed 2 years ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: Sylvestre, Assigned: Sylvestre)

References

Details

Attachments

(3 files, 2 obsolete files)

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:404:11: note: here
            default: {
            ^~~~~~~

I guess this is the same as bug 1426997.
Assignee: nobody → sledru
nsHtml5TreeBuilder.cpp is a generated filed, so the fix needs to be made in the HTML5 parser written in Java. See bug 1426997 comment 11 and 12.

Also, does this fallthrough case need to assert in debug builds? The HTML5 parser's fallthrough cases that I annotated were all intentional, so I used MOZ_FALLTHROUGH instead of MOZ_FALLTHROUGH_ASSERT. I wonder why this particular case was caught by gcc 8 but not by clang's -Wimplicit-fallthrough warning.
It appears that this line indeed in unreachable, so this would be r+ in that sense. However, recent experience indicates that if I say "r+, but please land the corresponding htmlparser repo fix", the htmlparser repo fix doesn't get landed. OTOH, I don't want to make people jump through the hoops of setting up HTML parser regeneration just to change one line. (Aside: Now that we can require Node to build Firefox, should we also require OpenJDK so as not to require special setup for the regeneration...)

Are you OK with doing the htmlparser repo landing or would you like me to do it?

To assert unreachability in Java, we can just say:

assert false;

which should translate into a MOZ_ASSERT. However, if gcc isn't happy enough with the specific kind of assertion that gets generated and still complains about fall-through, I guess we could say:

assert false;
// CPPONLY: MOZ_FALLTHROUGH_ASSERT("unreachable");
Flags: needinfo?(sledru)
I will do in the java parser too. No worries, thanks!
Flags: needinfo?(sledru)
Attached patch 1447480.diff (obsolete) — Splinter Review
The java change.
Attachment #8964596 - Flags: review?(hsivonen)
Attachment #8960801 - Attachment is obsolete: true
Attachment #8960801 - Flags: review?(hsivonen)
Attachment #8964596 - Flags: review?(hsivonen) → review+
Comment on attachment 8964597 [details]
Bug 1447480 - Add a MOZ_ASSERT to make it clear that it can never fall through

https://reviewboard.mozilla.org/r/233320/#review239076

::: parser/html/nsHtml5TreeBuilder.cpp:403
(Diff revision 1)
>                  reconstructTheActiveFormattingElements();
>                  continue;
>                }
>              }
> +            MOZ_ASSERT(false);
> +            MOZ_FALLTHROUGH_ASSERT(nsGkAtoms::unreachable);

Does this actually compile? The simplest way forward would be to use the macro without an argument. (And to make the macro support the no-argument case if not already supported.)
> ::: parser/html/nsHtml5TreeBuilder.cpp:403
> > +            MOZ_ASSERT(false);
> > +            MOZ_FALLTHROUGH_ASSERT(nsGkAtoms::unreachable);

MOZ_ASSERT(false) is redundant here. MOZ_FALLTHROUGH_ASSERT will (do the equivalent of) MOZ_ASSERT(false) in debug builds.
Attached patch foo2.diffSplinter Review
Attachment #8964596 - Attachment is obsolete: true
Attachment #8964822 - Flags: review?(hsivonen)
Attachment #8964822 - Flags: review?(hsivonen) → review+
Comment on attachment 8964597 [details]
Bug 1447480 - Add a MOZ_ASSERT to make it clear that it can never fall through

https://reviewboard.mozilla.org/r/233320/#review239180
Attachment #8964597 - Flags: review?(hsivonen) → review+
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fa109105e4ea
Add a MOZ_ASSERT to make it clear that it can never fall through r=hsivonen
https://hg.mozilla.org/projects/htmlparser/rev/2620240e520b0ea8a1b15d9a163c7d80dcf6c673
Bug 1447480 - Add a MOZ_ASSERT to make it clear that it can never fall through r?hsivonen
> Does this actually compile? The simplest way forward would be to use the
> macro without an argument. 
"not enough actual parameters for macro 'MOZ_FALLTHROUGH_ASSERT'"

Looks like it isn't :)
Flags: needinfo?(sledru)
Comment on attachment 8965253 [details]
Bug 1447480 - Add support of MOZ_FALLTHROUGH_ASSERT without any argument

https://reviewboard.mozilla.org/r/233964/#review239594

Seems reasonable to me, but I'm not officially a peer for this code.
Attachment #8965253 - Flags: review?(hsivonen) → review+
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a454ed4489f2
Add a MOZ_ASSERT to make it clear that it can never fall through r=hsivonen
https://hg.mozilla.org/integration/autoland/rev/5dfbd42ce515
Add support of MOZ_FALLTHROUGH_ASSERT without any argument r=hsivonen
Backout by toros@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e92d8ad7c1a0
Backed out 2 changesets for build bustages at dist/include/mozilla/Assertions.h:60 a=backout on a CLOSED TREE
Flags: needinfo?(sledru)
Attachment #8965253 - Flags: review+ → review?(hsivonen)
Comment on attachment 8965253 [details]
Bug 1447480 - Add support of MOZ_FALLTHROUGH_ASSERT without any argument

https://reviewboard.mozilla.org/r/233964/#review240530

I suppose the result is good enough even if not great.
Attachment #8965253 - Flags: review?(hsivonen) → review+
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/639322e83c65
Add a MOZ_ASSERT to make it clear that it can never fall through r=hsivonen
https://hg.mozilla.org/integration/autoland/rev/1fdb4dcb4225
Add support of MOZ_FALLTHROUGH_ASSERT without any argument r=hsivonen
https://hg.mozilla.org/mozilla-central/rev/639322e83c65
https://hg.mozilla.org/mozilla-central/rev/1fdb4dcb4225
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.