1236324 - Annotate intentional switch fallthroughs to suppress -Wimplicit-fallthrough warnings in toolkit/components/places/

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Chris Peterson [:cpeterson]]

Attachment: places_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.

toolkit/components/places/Database.cpp:182:5: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
toolkit/components/places/nsAnnotationService.cpp:215:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
toolkit/components/places/nsAnnotationService.cpp:227:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
toolkit/components/places/nsAnnotationService.cpp:297:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
toolkit/components/places/nsAnnotationService.cpp:309:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
toolkit/components/places/nsNavHistoryResult.cpp:2420:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
toolkit/components/places/nsNavHistoryResult.cpp:2446:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
toolkit/components/places/nsNavHistoryResult.cpp:2878:7 [-Wimplicit-fallthrough] unannotated fall-through between switch labels

Comment 1

[Marco Bonardo [:mak]]

Comment on attachment 8703400 [details] [diff] [review]
places_MOZ_FALLTHROUGH.patch

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

::: toolkit/components/places/nsNavHistoryResult.cpp
@@ +2440,5 @@
>                                                  query->EndTime());
>          if (aTime > endTime)
>            return NS_OK; // after our time range
>        }
>        // Now we know that our visit satisfies the time range, fallback to the

while here would you mind fixing this comment to say "fall through" instead of "fallback"?

Comment 2

[Chris Peterson [:cpeterson]]

(In reply to Marco Bonardo [::mak] from comment #1)
> while here would you mind fixing this comment to say "fall through" instead
> of "fallback"?

Thanks. I'll update the comment before landing.

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0556dfff69d3

Comment 4

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0556dfff69d3