Annotate intentional switch fallthroughs to suppress -Wimplicit-fallthrough warnings in parser/

RESOLVED FIXED in Firefox 46

Status

()

RESOLVED FIXED
3 years ago
9 months ago

People

(Reporter: cpeterson, Assigned: cpeterson)

Tracking

unspecified
mozilla46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

3 years ago
Created attachment 8703396 [details] [diff] [review]
parser_part-1_MOZ_FALLTHROUGH.patch

clang's -Wimplicit-fallthrough warnings (not yet enabled in mozilla-central) warn about switch cases that fall through without a break or return statement. MOZ_FALLTHROUGH (bug 1215411) is an annotation to suppress -Wimplicit-fallthrough warnings about switch cases that intentionally fall through without a break or return statement.  MOZ_FALLTHROUGH is only needed on cases that have code.

MOZ_FALLTHROUGH_ASSERT (bug 1235277) will suppress -Wimplicit-fallthrough warnings about switch cases that MOZ_ASSERT(false) (or its alias MOZ_ASSERT_UNREACHABLE) in debug builds, but intentionally fall through in release builds.

Why do we need MOZ_FALLTHROUGH_ASSERT in addition to MOZ_FALLTHROUGH? In release builds, the MOZ_ASSERT(false) will expand to `do { } while (0)`, requiring a MOZ_FALLTHROUGH annotation to suppress a -Wimplicit-fallthrough warning. In debug builds, the MOZ_ASSERT(false) will expand to something like `if (true) { MOZ_CRASH(); }` and the MOZ_FALLTHROUGH annotation will cause a -Wunreachable-code warning. The MOZ_FALLTHROUGH_ASSERT macro breaks this warning stalemate.

parser/html/nsHtml5Highlighter.cpp:572:9 [-Wimplicit-fallthrough] unannotated fall-through between switch labels

parser/htmlparser/nsScanner.cpp:425:7 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
parser/htmlparser/nsScanner.cpp:780:7 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
Attachment #8703396 - Flags: review?(hsivonen)
(Assignee)

Comment 1

3 years ago
Created attachment 8703397 [details] [diff] [review]
parser_part-2_Wno-implicit-fallthrough.patch

Part 2: Suppress -Wimplicit-fallthrough warnings in parser/html generated code.

parser/html/nsHtml5Tokenizer.cpp:456:13 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
parser/html/nsHtml5Tokenizer.cpp:463:7 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
parser/html/nsHtml5Tokenizer.cpp:524:7 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
parser/html/nsHtml5Tokenizer.cpp:540:13 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
parser/html/nsHtml5Tokenizer.cpp:563:13 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
parser/html/nsHtml5Tokenizer.cpp:574:7 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
parser/html/nsHtml5Tokenizer.cpp:592:13 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
parser/html/nsHtml5Tokenizer.cpp:611:13 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
parser/html/nsHtml5Tokenizer.cpp:619:13 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
parser/html/nsHtml5Tokenizer.cpp:631:7 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
parser/html/nsHtml5Tokenizer.cpp:647:13 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
...
Attachment #8703397 - Flags: review?(hsivonen)
(Assignee)

Updated

3 years ago
Depends on: 1215411
Attachment #8703396 - Flags: review?(hsivonen) → review+
Comment on attachment 8703397 [details] [diff] [review]
parser_part-2_Wno-implicit-fallthrough.patch

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

I've annotated implicit fallthroughs as comments in the Java source, so at some point as a follow-up, I should make the translator translate those into annotations in the C++
Attachment #8703397 - Flags: review?(hsivonen) → review+
(Assignee)

Comment 3

3 years ago
(In reply to Henri Sivonen (:hsivonen) from comment #2)
> I've annotated implicit fallthroughs as comments in the Java source, so at
> some point as a follow-up, I should make the translator translate those into
> annotations in the C++

Thanks. I'll look to add the annotations to the Java translator.

Comment 5

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/159e7916ff7d
https://hg.mozilla.org/mozilla-central/rev/462d202bc50d
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
(Assignee)

Updated

3 years ago
Blocks: 1253170
(Assignee)

Updated

9 months ago
Depends on: 1424548
(Assignee)

Comment 6

9 months ago
Bug 1236322 suppressed clang -Wimplicit-fallthrough warnings in the generated parser code because it did not include MOZ_FALLTHROUGH annotations. I updated the Java translator in bug 1424548.
You need to log in before you can comment on or make changes to this bug.