Fix -Wunreachable-code warnings in testplugin

RESOLVED FIXED in Firefox 47

Status

()

Core
Plug-ins
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: cpeterson, Assigned: cpeterson)

Tracking

unspecified
mozilla47
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
Created attachment 8724486 [details] [diff] [review]
Wunreachable-code_plugins.patch

dom/plugins/test/testplugin/nptest_macosx.mm:278:10: warning: will never be executed [-Wunreachable-code]
dom/plugins/test/testplugin/nptest_macosx.mm:307:10: warning: will never be executed [-Wunreachable-code]
dom/plugins/test/testplugin/nptest.cpp:697:7: warning: will never be executed [-Wunreachable-code]
dom/plugins/test/testplugin/nptest.cpp:2300:15: warning: will never be executed [-Wunreachable-code]
Attachment #8724486 - Flags: review?(jmathies)
(Assignee)

Updated

2 years ago
Blocks: 1235717
(Assignee)

Comment 1

2 years ago
Comment on attachment 8724486 [details] [diff] [review]
Wunreachable-code_plugins.patch

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

::: dom/plugins/test/testplugin/nptest.cpp
@@ +2204,5 @@
>          " expected " << var2->type;
>      return false;
>    }
>  
> +  switch (static_cast<int>(var1->type)) {

This switch statement on NPVariantType has cases for ever NPVariantType enum value, so clang rightly warns that the default case is unnecessary. I could have removed the default case, which should be safe to do because the compiler will warn if someone adds a new NPVariantType enum value without adding a corresponding case here. But since the default case returns a test failure, I thought the more conservative action might to case the NPVariantType value we're switching on to an int to suppress the compiler's checks for switching on an enum.

::: dom/plugins/test/testplugin/nptest_gtk2.cpp
@@ +471,5 @@
>    case EDGE_BOTTOM:
>      return pluginY + pluginHeight;
>    }
>  
> +  MOZ_CRASH("Unexpected RectEdge?!");

This statement is unreachable because the switch above (and those below) handle all EDGE_* enum values. If someone adds a new EDGE_* enum value without adding a corresponding case here, the compiler will warn that this switch no longer handles every EDGE_* value.

Comment 2

2 years ago
(In reply to Chris Peterson [:cpeterson] from comment #1)
> Comment on attachment 8724486 [details] [diff] [review]
> Wunreachable-code_plugins.patch
> 
> Review of attachment 8724486 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/plugins/test/testplugin/nptest.cpp
> @@ +2204,5 @@
> >          " expected " << var2->type;
> >      return false;
> >    }
> >  
> > +  switch (static_cast<int>(var1->type)) {
> 
> This switch statement on NPVariantType has cases for ever NPVariantType enum
> value, so clang rightly warns that the default case is unnecessary. I could
> have removed the default case, which should be safe to do because the
> compiler will warn if someone adds a new NPVariantType enum value without
> adding a corresponding case here. But since the default case returns a test
> failure, I thought the more conservative action might to case the
> NPVariantType value we're switching on to an int to suppress the compiler's
> checks for switching on an enum.

Lets add a comment here about this so people don't mess with it.

Updated

2 years ago
Attachment #8724486 - Flags: review?(jmathies) → review+
(Assignee)

Comment 3

2 years ago
Thanks, Jim. I'll add a comment.

Comment 5

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a1800ff23514
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.