Fix -Wimplicit-fallthrough warnings in layout/

RESOLVED FIXED in Firefox 46

Status

()

defect
RESOLVED FIXED
4 years ago
11 months ago

People

(Reporter: cpeterson, Assigned: cpeterson)

Tracking

unspecified
mozilla46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(1 attachment)

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

layout/base/nsCSSRendering.cpp:3913:3 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/base/nsCSSRendering.cpp:3943:3 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/base/nsCSSRendering.cpp:4066:3 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/base/nsCSSRendering.cpp:4096:3 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/base/nsCSSRenderingBorders.cpp:646:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/base/nsLayoutUtils.cpp:4639:9 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/base/nsLayoutUtils.cpp:4659:9 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/base/nsLayoutUtils.cpp:5004:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/base/nsLayoutUtils.cpp:5200:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/base/TouchManager.cpp:192:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/base/TouchManager.cpp:196:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/generic/nsFlexContainerFrame.cpp:2497:7 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/generic/nsFlexContainerFrame.cpp:2687:7 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/generic/nsFlexContainerFrame.cpp:2973:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/generic/nsFrame.cpp:4277:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/generic/nsFrame.cpp:4310:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/generic/nsFrame.cpp:4313:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/generic/nsFrame.cpp:6703:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/generic/nsFrame.cpp:6751:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/generic/nsGridContainerFrame.cpp:2649:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/generic/nsGridContainerFrame.cpp:935:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/generic/nsHTMLReflowState.cpp:1141:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/generic/nsHTMLReflowState.cpp:1145:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/generic/nsHTMLReflowState.cpp:1148:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/generic/nsLineLayout.cpp:2942:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/generic/nsLineLayout.cpp:2958:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/generic/nsLineLayout.cpp:3134:7 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/generic/nsLineLayout.cpp:3150:7 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/printing/nsPrintPreviewListener.cpp:199:7 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/style/CSSLexer.cpp:129:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/style/Declaration.cpp:1069:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/style/Declaration.cpp:366:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/style/Declaration.cpp:442:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/style/Declaration.cpp:981:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/style/nsComputedDOMStyle.cpp:3597:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/style/nsComputedDOMStyle.cpp:3616:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/style/nsComputedDOMStyle.cpp:539:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/style/nsComputedDOMStyle.cpp:540:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/style/nsComputedDOMStyle.cpp:542:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/style/nsCSSParser.cpp:10628:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/style/nsCSSParser.cpp:10630:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/style/nsCSSParser.cpp:10671:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/style/nsCSSParser.cpp:10673:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/style/nsCSSParser.cpp:10769:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/style/nsCSSParser.cpp:10770:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/style/nsCSSParser.cpp:10774:43 [-Wimplicit-fallthrough] fallthrough annotation does not directly precede switch label
layout/style/nsCSSParser.cpp:10775:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/style/nsCSSParser.cpp:10776:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/style/nsCSSParser.cpp:10780:43 [-Wimplicit-fallthrough] fallthrough annotation does not directly precede switch label
layout/style/nsCSSParser.cpp:2542:7 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/style/nsCSSParser.cpp:2715:7 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/style/nsCSSParser.cpp:4124:7 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/style/nsCSSParser.cpp:4313:7 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/style/nsCSSParser.cpp:9513:3 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/style/nsCSSParser.cpp:9697:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/style/nsCSSParser.cpp:9699:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/style/nsCSSParser.cpp:9743:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/style/nsCSSParser.cpp:9745:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/style/nsCSSParser.cpp:9826:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/style/nsCSSParser.cpp:9827:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/style/nsCSSParser.cpp:9832:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/style/nsCSSParser.cpp:9833:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/style/nsCSSParser.cpp:9980:3 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/style/nsRuleNode.cpp:160:3 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/style/nsRuleNode.cpp:187:3 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/style/nsRuleNode.cpp:722:3 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/style/nsRuleNode.cpp:753:3 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/style/StyleAnimationValue.cpp:139:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/style/StyleAnimationValue.cpp:1687:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/style/StyleAnimationValue.cpp:1869:7 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/tables/FixedTableLayoutStrategy.cpp:264:13 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/tables/FixedTableLayoutStrategy.cpp:267:13 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/tables/nsCellMap.cpp:1043:3 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/tables/nsCellMap.cpp:930:3 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/tables/nsCellMap.cpp:953:3 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/tables/nsCellMap.cpp:997:3 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/tables/nsTableFrame.cpp:6943:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/tables/nsTableFrame.cpp:6953:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/tables/nsTableFrame.cpp:6959:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/tables/nsTableFrame.cpp:6966:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/tables/nsTableFrame.cpp:6974:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/tables/nsTableFrame.cpp:7151:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/tables/nsTableFrame.cpp:7161:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/tables/nsTableFrame.cpp:7170:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/tables/nsTableFrame.cpp:7177:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/tables/nsTableFrame.cpp:7186:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/tables/nsTableRowFrame.cpp:663:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/tables/SpanningCellSorter.cpp:112:9 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/tables/SpanningCellSorter.cpp:142:9 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/tables/SpanningCellSorter.cpp:157:9 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/xul/nsResizerFrame.cpp:86:13 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/xul/nsResizerFrame.cpp:87:13 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/xul/nsResizerFrame.cpp:88:13 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/xul/nsResizerFrame.cpp:90:13 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/xul/nsSliderFrame.cpp:551:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/xul/nsSliderFrame.cpp:560:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
layout/xul/nsXULPopupManager.cpp:2268:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
Attachment #8702201 - Flags: review?(dholbert)
Comment on attachment 8702201 [details] [diff] [review]
layout_MOZ_FALLTHROUGH.patch

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

r=me with nit below (about too-long-line) addressed.

::: layout/style/nsCSSParser.cpp
@@ +10896,5 @@
>    // Provide missing values by replicating some of the values found
>    switch (countX) {
> +    case 1: dimenX.mRight = dimenX.mTop;  MOZ_FALLTHROUGH;  // top-right same as top-left, and
> +    case 2: dimenX.mBottom = dimenX.mTop; MOZ_FALLTHROUGH;  // bottom-right same as top-left, and
> +    case 3: dimenX.mLeft = dimenX.mRight;                   // bottom-left same as top-right

These lines end up too long (much more than 80 columns). Could you rewrap?

Might be best to just format this like the case statement directly above it (in ParseGroupedBoxProperty). The one-liner format made sense when there was only one statement per case here, but it doesn't really work with the MOZ_FALLTHROUGH bulk.

@@ +10902,5 @@
>  
>    switch (countY) {
> +    case 1: dimenY.mRight = dimenY.mTop;  MOZ_FALLTHROUGH;  // top-right same as top-left, and
> +    case 2: dimenY.mBottom = dimenY.mTop; MOZ_FALLTHROUGH;  // bottom-right same as top-left, and
> +    case 3: dimenY.mLeft = dimenY.mRight;                   // bottom-left same as top-right

Same here.

::: layout/style/nsRuleNode.cpp
@@ +751,5 @@
>    case NS_STYLE_BG_POSITION_RIGHT:
>    case NS_STYLE_BG_POSITION_BOTTOM:
>      return 1.0f;
>    default:
> +    MOZ_FALLTHROUGH_ASSERT("unexpected box position value");

(no action needed here)
Looks like you're promoting this from being a nonfatal assertion to being a fatal assertion. That's not always a clearly-desirable change[1], but I think it's reasonable in this case.

[1] http://robert.ocallahan.org/2011/12/case-for-non-fatal-assertions.html

::: layout/tables/nsTableFrame.cpp
@@ +7000,5 @@
>      case eColOwner:
>        owner = col;
>        break;
>      case eAjaRowGroupOwner:
> +      MOZ_FALLTHROUGH_ASSERT("a neighboring rowgroup can never own a vertical border");

(no action needed here)
I'm not sure that this nonfatal-->fatal assertion-upgrade makes sense (kinda depends on how likely this is to happen & how bad it is when it does happen).

But I suppose we can always revert to nonfatal later on if this is problematic, so this seems fine.
Attachment #8702201 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #1)
> These lines end up too long (much more than 80 columns). Could you rewrap?

Thanks for catching this. I'll fix these.


> ::: layout/style/nsRuleNode.cpp
> >    default:
> > +    MOZ_FALLTHROUGH_ASSERT("unexpected box position value");
> 
> (no action needed here)
> Looks like you're promoting this from being a nonfatal assertion to being a
> fatal assertion. That's not always a clearly-desirable change[1], but I
> think it's reasonable in this case.
> 
> [1] http://robert.ocallahan.org/2011/12/case-for-non-fatal-assertions.html

Thanks for the link. roc's point about non-fatal assertions and bug priorities makes sense.

I knew NS_ERROR() and NS_NOTREACHED() called NS_DebugBreak(NS_DEBUG_ASSERTION), but I didn't realize NS_DEBUG_ASSERTION was non-fatal. In this particular case, I think a fatal assertion is reasonable because it would clearly indicate a real bug.


> ::: layout/tables/nsTableFrame.cpp
> @@ +7000,5 @@
> >      case eColOwner:
> >        owner = col;
> >        break;
> >      case eAjaRowGroupOwner:
> > +      MOZ_FALLTHROUGH_ASSERT("a neighboring rowgroup can never own a vertical border");
> 
> (no action needed here)
> I'm not sure that this nonfatal-->fatal assertion-upgrade makes sense (kinda
> depends on how likely this is to happen & how bad it is when it does happen).

I will revert this change. I don't know how likely or bad this NS_ERROR() might be to gauge whether it should be fatal.
Sounds good, thanks!

In that case, please also revert the "program error" assertion a bit below that one, too (under a eAjaRowOwner: case statement, a few lines further down in nsTableFrame.cpp).  I have similar uncertainty about how bad that assertion is & whether it merits the upgrade to be fatal.
Comment on attachment 8702201 [details] [diff] [review]
layout_MOZ_FALLTHROUGH.patch

>+++ b/layout/style/nsCSSParser.cpp
>@@ -2726,17 +2726,17 @@ CSSParserImpl::ResolveValueWithVariableR
>       default:
>         NS_NOTREACHED("unexpected token type");
>-        // fall through
>+        MOZ_FALLTHROUGH;
>       case eCSSToken_ID:
>       case eCSSToken_String:
>       case eCSSToken_Includes:
>       case eCSSToken_Beginsmatch:
>       case eCSSToken_Endsmatch:
>         UPDATE_RECORDING_TOKENS(eCSSTokenSerialization_Other);
>         break;

Looks like you upgraded this (nonfatal) NS_NOTREACHED invocatoin to be fatal (MOZ_FALLTHROUGH_ASSERT) in the cset that ended up landing.

As in the other cases noted in comment 1, I'm not sure that's wise.  But I suppose it's fine; we can revert it later if necessary.
https://hg.mozilla.org/mozilla-central/rev/aab78aed1c92
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Blocks: 1253170
Product: Core → Core Graveyard
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.