1253753 - Remove unnecessary switch fallthrough to avoid -Wimplicit-fallthrough warning

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Chris Peterson [:cpeterson]]

Attachment: Wimplicit-fallthrough_NativeKeyBindings.cpp

clang's -Wimplicit-fallthrough warning reports switch cases that fall through without a break or return statement. These warnings are not yet enabled on mozilla-central but they will be (in bug 1253170) after this final -Wimplicit-fallthrough warning is fixed:

widget/gtk/NativeKeyBindings.cpp:230:5: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]

MOZ_FALLTHROUGH (bug 1215411) is an annotation to suppress -Wimplicit-fallthrough warnings where the fallthrough is intentional. But we don't need the MOZ_FALLTHROUGH annotation in this case. We can remove the intentional fallthrough because the `default` case is not needed. The switch already handles all three enum values of NativeKeyBindingsType:

https://mxr.mozilla.org/mozilla-central/source/widget/nsIWidget.h#1881

An additional benefit of removing the unnecessary default case is that the compiler can then warn (gcc -Wswitch) if someone adds a fourth enum value to NativeKeyBindingsType but forgets to add a case for it in NativeKeyBindings::GetInstance().

Comment 1

[Karl Tomlinson (:karlt)]

Comment on attachment 8726956 [details] [diff] [review]
Wimplicit-fallthrough_NativeKeyBindings.cpp

> An additional benefit of removing the unnecessary default case is that the
> compiler can then warn (gcc -Wswitch) if someone adds a fourth enum value to
> NativeKeyBindingsType but forgets to add a case for it in
> NativeKeyBindings::GetInstance().

That sounds a helpful warning (and much better than -Wswitch-enum), but we
don't have that enabled yet, and so there is still the possibility of someone
adding another value.

Getting the wrong kind of NativeKeyBindings is not the kind of failure that
means a crash is preferable, so I'm not happy with the crash here.

I prefer MOZ_FALLTHROUGH() for now at least.

I also wonder whether we can be sure not to get warnings for the missing
return value in the MOZ_CRASH path.

Even once we know we have -Werror -Wswitch, the compiler will still not know
that a value is one of the enumerators when it belongs to "an enumeration that
has values not defined by any of its enumerators", and so a fallback path will
still be required.  At least then perhaps we can argue that the benefit of
-Wswitch outweighs the risk of crash when the value is not one of the
enumerators.  But we will be able to get the warning without the crash with

   NativeKeyBindings** instance = &sInstanceForMultiLineEditor;
   switch (aType) {
     case nsIWidget::NativeKeyBindingsForSingleLineEditor:
       instance = &sInstanceForSingleLineEditor;
       break;
 
     case nsIWidget::NativeKeyBindingsForMultiLineEditor:
     case nsIWidget::NativeKeyBindingsForRichTextEditor:
   }

   if (!*instance) {
     *instance = new NativeKeyBindings();
     (*instance)->Init(aType);
   }
   return *instance;

Comment 2

[Chris Peterson [:cpeterson]]

Attachment: MOZ_FALLTHROUGH-NativeKeyBindings-v2.patch

(In reply to Karl Tomlinson (ni?:karlt) from comment #1)
> That sounds a helpful warning (and much better than -Wswitch-enum), but we
> don't have that enabled yet, and so there is still the possibility of someone
> adding another value.

-Wswitch is already enabled by -Wall on both clang and gcc:

https://gcc.gnu.org/onlinedocs/gcc-4.9.0/gcc/Warning-Options.html#Warning-Options

> Getting the wrong kind of NativeKeyBindings is not the kind of failure that
> means a crash is preferable, so I'm not happy with the crash here.
> 
> I prefer MOZ_FALLTHROUGH() for now at least.

Sure. Here's a patch that just replaces the MOZ_ASSERT with a MOZ_FALLTHROUGH_ASSERT, a variant of MOZ_FALLTHROUGH that asserts in debug builds and suppresses the warning in release builds.

-Wswitch warnings are nice, but the assert is adequate to catch a missing case in GetInstance() if someone adds a new enum value.

Comment 3

[Karl Tomlinson (:karlt)]

Comment on attachment 8727250 [details] [diff] [review]
MOZ_FALLTHROUGH-NativeKeyBindings-v2.patch

Seems like this is the wrong patch, but MOZ_FALLTHROUGH_ASSERT sounds good, thanks.

Good to know that -Wswitch is already enabled thanks.  If we have warnings as errors in this subdir and the missing return statement is added or explained, then I'd be OK with the first patch too.  I think I prefer MOZ_FALLTHROUGH_ASSERT, but either is OK, thanks.

Comment 4

[Chris Peterson [:cpeterson]]

(In reply to Karl Tomlinson (ni?:karlt) from comment #3)
> Seems like this is the wrong patch, but MOZ_FALLTHROUGH_ASSERT sounds good,
> thanks.

Sorry! I forgot to hg qrefresh my local changes. I'll land the MOZ_FALLTHROUGH_ASSERT patch; it is a one-line macro change.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a692374920e8

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a692374920e8