1570499 - (fallthrough) Replace MOZ_FALLTHROUGH macro with C++17 attribute [[fallthrough]]

Status: RESOLVED FIXED

(Core :: MFBT, task, P3)

Description

[Chris Peterson [:cpeterson]]

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.

Comment 1

[Chris Peterson [:cpeterson]]

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).

Comment 2

[Chris Peterson [:cpeterson]]

Green Try build:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8617da3b8cf2861a324b628e53c5f6e690518b67

Comment 3

[Chris Peterson [:cpeterson]]

Attachment: Bug 1570499 - Part 1: Replace MOZ_FALLTHROUGH macro with C++17's [[fallthrough]] attribute. r?froydnj

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.

Comment 4

[Chris Peterson [:cpeterson]]

Attachment: Bug 1570499 - Part 2: Suppress -Wimplicit-fallthrough warnings from third-party udis86 code. r?froydnj

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

Comment 5

[Chris Peterson [:cpeterson]]

Attachment: Bug 1570499 - Part 3: Remove MOZ_FALLTHROUGH macro definition. r?froydnj

Depends on D56441

Comment 6

[Pulsebot]

Pushed by [e-mail]:
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

Comment 7

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/68b0f6bd38ad
https://hg.mozilla.org/mozilla-central/rev/1802196cadec
https://hg.mozilla.org/mozilla-central/rev/221bb3c0cea1