Last Comment Bug 723534 - Remove dead default cases.
: Remove dead default cases.
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla13
Assigned To: Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
:
:
Mentors:
Depends on: 723114
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-02 08:09 PST by Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
Modified: 2012-02-25 02:16 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Remove dead default cases. (4.43 KB, patch)
2012-02-02 08:09 PST, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
no flags Details | Diff | Splinter Review
Remove dead default cases. (1.37 KB, patch)
2012-02-03 01:43 PST, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
ehsan: review+
Details | Diff | Splinter Review

Description Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-02-02 08:09:22 PST
Created attachment 593846 [details] [diff] [review]
Remove dead default cases.

Clang warns on default cases that are never reached. This is currently breaking the build with --enable-warnings-as-errors. The attached patch fixes some cases.
Comment 1 Jeff Muizelaar [:jrmuizel] 2012-02-02 08:11:19 PST
Comment on attachment 593846 [details] [diff] [review]
Remove dead default cases.

This patch is a bit riskier, let's get Ehsan to review it as his probably a more appropriate reviewer for stuff in content/.
Comment 2 :Ehsan Akhgari 2012-02-02 11:09:55 PST
Comment on attachment 593846 [details] [diff] [review]
Remove dead default cases.

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

I'm not the right person to review the canvas changes.

::: content/smil/nsSMILTimedElement.cpp
@@ +2203,2 @@
>    }
> +  MOZ_NOT_REACHED("Switch is incomplete!");

Please don't change the debug string here.  Also shouldn't you return something here?

@@ +2275,2 @@
>    }
> +  MOZ_NOT_REACHED("Switch is incomplete!");

Ditto.
Comment 3 Jeff Muizelaar [:jrmuizel] 2012-02-02 14:04:52 PST
Comment on attachment 593846 [details] [diff] [review]
Remove dead default cases.

The canvas changes seem fine to me.
Comment 4 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-02-03 01:30:46 PST
(In reply to Ehsan Akhgari [:ehsan] from comment #2)
> Comment on attachment 593846 [details] [diff] [review]
> Remove dead default cases.
> 
> Review of attachment 593846 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not the right person to review the canvas changes.
> 
> ::: content/smil/nsSMILTimedElement.cpp
> @@ +2203,2 @@
> >    }
> > +  MOZ_NOT_REACHED("Switch is incomplete!");
> 
> Please don't change the debug string here.  Also shouldn't you return
> something here?

No, that is the point. The dynamics are:

* We build with -Werror
* If the switch is missing a case and has no default, the build fails.
* If the switch has all the cases and a default, the build succeeds

The net result is that without the default case, the build works if and only if all the cases ore covered, which is normally exactly what we need. The unreachable is there just to help other compilers and as documentation.
Comment 5 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-02-03 01:43:14 PST
Created attachment 594091 [details] [diff] [review]
Remove dead default cases.
Comment 6 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-02-24 05:38:31 PST
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=038c62997c55
Comment 7 Marco Bonardo [::mak] 2012-02-25 02:16:57 PST
https://hg.mozilla.org/mozilla-central/rev/038c62997c55

Note You need to log in before you can comment on or make changes to this bug.