Fix build warnings in content/media

RESOLVED FIXED in mozilla15

Status

()

Core
Audio/Video
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Ms2ger, Assigned: Ms2ger)

Tracking

(Blocks: 1 bug)

Trunk
mozilla15
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Created attachment 623480 [details] [diff] [review]
Patch v1

It is time.
Attachment #623480 - Flags: review?(cpearce)

Comment 1

5 years ago
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?
(Assignee)

Comment 2

5 years ago
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)
(Assignee)

Comment 4

5 years ago
(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.

Comment 7

5 years ago
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?
(Assignee)

Comment 8

5 years ago
Yes, see comment 5, unless you opt in, this doesn't affect your local builds...
Attachment #623480 - Flags: feedback?(roc) → feedback+
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 11

5 years ago
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)
(Assignee)

Comment 12

5 years ago
Created attachment 623953 [details] [diff] [review]
Patch v2
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).
(Assignee)

Updated

5 years ago
Blocks: 756397
(Assignee)

Comment 16

5 years ago
https://hg.mozilla.org/mozilla-central/rev/358a6096a0da
Status: ASSIGNED → RESOLVED
Last Resolved: 5 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.