Closed Bug 1570499 (fallthrough) Opened 2 years ago Closed 1 year ago

Replace MOZ_FALLTHROUGH macro with C++17 attribute [[fallthrough]]

Categories

(Core :: MFBT, task, P3)

task

Tracking

()

RESOLVED FIXED
mozilla73
Tracking Status
firefox-esr68 --- wontfix
firefox70 --- wontfix
firefox71 --- wontfix
firefox72 --- wontfix
firefox73 --- fixed

People

(Reporter: cpeterson, Assigned: cpeterson)

References

Details

Attachments

(3 files)

Bug 1215411 defined MOZ_FALLTHROUGH using clang's nonstandard attribute [[clang::fallthrough]]. After we compile as C++17 by default (bug 1560664), we can replace MOZ_FALLTHROUGH with C++17's standard [[fallthrough]] attribute.

https://en.cppreference.com/w/cpp/language/attributes/fallthrough

TBD whether we can then remove the MOZ_FALLTHROUGH_ASSERT macro (from bug 1235277) that works around clang bug https://llvm.org/bugs/show_bug.cgi?id=25966.

TBD whether we can then remove the MOZ_FALLTHROUGH_ASSERT macro (from bug 1235277) that works around clang bug https://llvm.org/bugs/show_bug.cgi?id=25966.

Unfortunately, the MOZ_FALLTHROUGH_ASSERT macro to assert on case fallthrough is still necessary after switching from [[clang::fallthrough]] to [[fallthrough]] because:

  • MOZ_ASSERT(false) followed by [[fallthrough]] causes a -Wunreachable-code warning in DEBUG builds
  • but MOZ_ASSERT(false) without [[fallthrough]] causes a -Wimplicit-fallthrough warning in NDEBUG builds).
Assignee: nobody → cpeterson

This changeset is a simple find and replace of MOZ_FALLTHROUGH and [[fallthrough]].

Unfortunately, the MOZ_FALLTHROUGH_ASSERT macro (to assert on case fallthrough in debug builds) is still necessary after switching from [[clang::fallthrough]] to [[fallthrough]] because:

  • MOZ_ASSERT(false) followed by [[fallthrough]] triggers a -Wunreachable-code warning in DEBUG builds
  • but MOZ_ASSERT(false) without [[fallthrough]] triggers a -Wimplicit-fallthrough warning in NDEBUG builds.

ProcessRedirect.cpp includes third-party udis86 C code that triggers -Wimplicit-fallthrough warnings. We suppress these warnings in ProcessRedirect.cpp because we want to minimize Mozilla changes to third-party code and we can't use C++17's [[fallthrough]] attribute in C code anyway. We don't suppress the warnings for the entire ProcessRedirect.cpp file (e.g. in moz.build) because we'd like clang -Wimplicit-fallthrough to check ProcessRedirect.cpp's own use of [[fallthrough]].

This changeset reverts some earlier Mozilla changes [1] made to upstream udis86's decode.c [2] that are no longer necessary.

[1] https://hg.mozilla.org/mozilla-central/rev/9042673fb235c00fbb021ea6356f4b0921505d1d
[2] https://github.com/vmt/udis86/blob/master/libudis86/decode.c#L747

Depends on D56440

Depends on: 1603919
Pushed by cpeterson@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/68b0f6bd38ad
Part 1: Replace MOZ_FALLTHROUGH macro with C++17's [[fallthrough]] attribute. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/1802196cadec
Part 2: Suppress -Wimplicit-fallthrough warnings from third-party udis86 code. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/221bb3c0cea1
Part 3: Remove MOZ_FALLTHROUGH macro definition. r=froydnj
Regressions: 1605410
Blocks: 1605436
You need to log in before you can comment on or make changes to this bug.