Closed Bug 754643 Opened 12 years ago Closed 12 years ago

Fix build warnings in content/media

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: Ms2ger, Assigned: Ms2ger)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Patch v1 (obsolete) — Splinter Review
It is time.
Attachment #623480 - Flags: review?(cpearce)
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?
I guess I could, but I think that makes more sense if the right-hand side has side effects.
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
Attachment #623480 - Flags: feedback?(roc)
Attachment #623480 - Flags: feedback?(kinetik)
Attachment #623480 - Flags: feedback?(chris.double)
(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.
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.
(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.
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?
Yes, see comment 5, unless you opt in, this doesn't affect your local builds...
Comment on attachment 623480 [details] [diff] [review]
Patch v1

I think this crusade is misguided, but I won't stand in its way.
Attachment #623480 - Flags: feedback?(kinetik)
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+.
Attachment #623480 - Flags: review?(cpearce) → review-
Comment on attachment 623480 [details] [diff] [review]
Patch v1

Not a fan of warnings as errors myself but won't hold it back.
Attachment #623480 - Flags: feedback?(chris.double)
Attached patch Patch v2Splinter Review
Attachment #623480 - Attachment is obsolete: true
Attachment #623953 - Flags: review?(cpearce)
Comment on attachment 623953 [details] [diff] [review]
Patch v2

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

Thanks.
Attachment #623953 - Flags: review?(cpearce) → review+
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.
(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).
Blocks: 756397
https://hg.mozilla.org/mozilla-central/rev/358a6096a0da
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Summary: Enable FAIL_ON_WARNINGS in content/media → Fix build warnings in content/media
Target Milestone: --- → mozilla15
You need to log in before you can comment on or make changes to this bug.