Last Comment Bug 754643 - Fix build warnings in content/media
: Fix build warnings in content/media
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: :Ms2ger (⌚ UTC+1/+2)
:
: Maire Reavy [:mreavy]
Mentors:
Depends on:
Blocks: buildwarning 756397
  Show dependency treegraph
 
Reported: 2012-05-13 00:34 PDT by :Ms2ger (⌚ UTC+1/+2)
Modified: 2012-05-18 02:48 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (12.61 KB, patch)
2012-05-13 00:34 PDT, :Ms2ger (⌚ UTC+1/+2)
cpearce: review-
roc: feedback+
Details | Diff | Splinter Review
Patch v2 (11.71 KB, patch)
2012-05-15 00:08 PDT, :Ms2ger (⌚ UTC+1/+2)
cpearce: review+
Details | Diff | Splinter Review

Description :Ms2ger (⌚ UTC+1/+2) 2012-05-13 00:34:37 PDT
Created attachment 623480 [details] [diff] [review]
Patch v1

It is time.
Comment 1 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-05-13 01:54:15 PDT
Comment on attachment 623480 [details] [diff] [review]
Patch v1

>+#ifdef DEBUG
>   NS_ASSERTION(mUnstamped.Length() > 0, "Must have unstamped packets");
>   ogg_packet* last = mUnstamped[mUnstamped.Length()-1];
>   NS_ASSERTION(last->e_o_s || last->granulepos > 0,
>       "Must know last granulepos!");
>+#endif

Can't you just use DebugOnly here?
Comment 2 :Ms2ger (⌚ UTC+1/+2) 2012-05-13 02:34:17 PDT
I guess I could, but I think that makes more sense if the right-hand side has side effects.
Comment 3 Chris Pearce (:cpearce) 2012-05-13 03:08:19 PDT
Comment on attachment 623480 [details] [diff] [review]
Patch v1

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

I'm not opposed to this change, but I think there should be consensus from the core team before we do this.

::: content/media/ogg/nsOggCodecState.cpp
@@ +924,2 @@
>    NS_ASSERTION(mUnstamped.Length() > 0, "Must have unstamped packets");
>    ogg_packet* last = mUnstamped[mUnstamped.Length()-1];

|last| is only used in debug code, so make it DebugOnly<last> instead of using ifdefs.

::: xpcom/glue/nsCycleCollectionParticipant.h
@@ +382,5 @@
>  #define NS_IMPL_CYCLE_COLLECTION_UNLINK_NSTARRAY(_field)                       \
>      tmp->_field.Clear();
>  
>  #define NS_IMPL_CYCLE_COLLECTION_UNLINK_END                                    \
> +    (void)tmp;                                                                 \

The evolving convention seems to be to << unused parameters to mozilla::unused., e.g.: 
http://mxr.mozilla.org/mozilla-central/source/extensions/cookie/nsPermissionManager.cpp#455
Comment 4 :Ms2ger (⌚ UTC+1/+2) 2012-05-13 03:15:08 PDT
(In reply to Chris Pearce (:cpearce) from comment #3)
> Comment on attachment 623480 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 623480 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not opposed to this change, but I think there should be consensus from
> the core team before we do this.
> 
> ::: content/media/ogg/nsOggCodecState.cpp
> @@ +924,2 @@
> >    NS_ASSERTION(mUnstamped.Length() > 0, "Must have unstamped packets");
> >    ogg_packet* last = mUnstamped[mUnstamped.Length()-1];
> 
> |last| is only used in debug code, so make it DebugOnly<last> instead of
> using ifdefs.

See above.

> ::: xpcom/glue/nsCycleCollectionParticipant.h
> @@ +382,5 @@
> >  #define NS_IMPL_CYCLE_COLLECTION_UNLINK_NSTARRAY(_field)                       \
> >      tmp->_field.Clear();
> >  
> >  #define NS_IMPL_CYCLE_COLLECTION_UNLINK_END                                    \
> > +    (void)tmp;                                                                 \
> 
> The evolving convention seems to be to << unused parameters to
> mozilla::unused., e.g.: 
> http://mxr.mozilla.org/mozilla-central/source/extensions/cookie/
> nsPermissionManager.cpp#455

Yes, but this header has (void)tmp; in a lot of places. Changing the convention here is out of scope for this bug.
Comment 5 Chris Pearce (:cpearce) 2012-05-13 03:27:21 PDT
Note this requires "ac_add_options --enable-warnings-as-errors" to take effect, so this will only cause build failures on weird configurations if the builder has opted in.
Comment 6 Chris Pearce (:cpearce) 2012-05-13 03:29:26 PDT
(In reply to Ms2ger from comment #4)
> (In reply to Chris Pearce (:cpearce) from comment #3)
> > Comment on attachment 623480 [details] [diff] [review]
> > Patch v1
> > 
> > Review of attachment 623480 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I'm not opposed to this change, but I think there should be consensus from
> > the core team before we do this.
> > 
> > ::: content/media/ogg/nsOggCodecState.cpp
> > @@ +924,2 @@
> > >    NS_ASSERTION(mUnstamped.Length() > 0, "Must have unstamped packets");
> > >    ogg_packet* last = mUnstamped[mUnstamped.Length()-1];
> > 
> > |last| is only used in debug code, so make it DebugOnly<last> instead of
> > using ifdefs.
> 
> See above.

I have seen above and I agree.

> > ::: xpcom/glue/nsCycleCollectionParticipant.h
> > @@ +382,5 @@
> > >  #define NS_IMPL_CYCLE_COLLECTION_UNLINK_NSTARRAY(_field)                       \
> > >      tmp->_field.Clear();
> > >  
> > >  #define NS_IMPL_CYCLE_COLLECTION_UNLINK_END                                    \
> > > +    (void)tmp;                                                                 \
> > 
> > The evolving convention seems to be to << unused parameters to
> > mozilla::unused., e.g.: 
> > http://mxr.mozilla.org/mozilla-central/source/extensions/cookie/
> > nsPermissionManager.cpp#455
> 
> Yes, but this header has (void)tmp; in a lot of places. Changing the
> convention here is out of scope for this bug.

Fair enough.
Comment 7 cajbir (:cajbir) 2012-05-13 03:43:26 PDT
The last time I remember fail on warnings being done I was unable to build firefox due to using a different GCC version to most developers and it having new warnings. How has this type of thing been resolved?
Comment 8 :Ms2ger (⌚ UTC+1/+2) 2012-05-13 03:49:56 PDT
Yes, see comment 5, unless you opt in, this doesn't affect your local builds...
Comment 9 Matthew Gregan [:kinetik] 2012-05-13 16:11:09 PDT
Comment on attachment 623480 [details] [diff] [review]
Patch v1

I think this crusade is misguided, but I won't stand in its way.
Comment 10 Chris Pearce (:cpearce) 2012-05-14 14:49:57 PDT
Comment on attachment 623480 [details] [diff] [review]
Patch v1

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

There doesn't seem to be strong opposition to this, so address the DebugOnly<> review comment and I'll r+.
Comment 11 cajbir (:cajbir) 2012-05-14 17:35:30 PDT
Comment on attachment 623480 [details] [diff] [review]
Patch v1

Not a fan of warnings as errors myself but won't hold it back.
Comment 12 :Ms2ger (⌚ UTC+1/+2) 2012-05-15 00:08:02 PDT
Created attachment 623953 [details] [diff] [review]
Patch v2
Comment 13 Chris Pearce (:cpearce) 2012-05-15 01:22:02 PDT
Comment on attachment 623953 [details] [diff] [review]
Patch v2

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

Thanks.
Comment 14 Ralph Giles (:rillian) needinfo me 2012-05-15 09:56:22 PDT
I object to -Werror being on by default. I hate being interrupted to fix someone else's warnings in the middle of working on my own patch, and it's off-putting for new contributors who happen to be using a different compiler version than the developers.
Comment 15 Timothy B. Terriberry (:derf) 2012-05-15 09:59:10 PDT
(In reply to Ralph Giles (:rillian) from comment #14)
> I object to -Werror being on by default. I hate being interrupted to fix
> someone else's warnings in the middle of working on my own patch, and it's
> off-putting for new contributors who happen to be using a different compiler
> version than the developers.

As mentioned in comment 5, you have to opt-in for this to affect your own local builds. Which just means that the problems won't show up until you run the patch through try (already a problem if you're using a different compiler/toolchain).
Comment 16 :Ms2ger (⌚ UTC+1/+2) 2012-05-18 02:48:15 PDT
https://hg.mozilla.org/mozilla-central/rev/358a6096a0da

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