Closed
Bug 754643
Opened 12 years ago
Closed 12 years ago
Fix build warnings in content/media
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: Ms2ger, Assigned: Ms2ger)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
11.71 KB,
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
It is time.
Attachment #623480 -
Flags: review?(cpearce)
Comment 1•12 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•12 years ago
|
||
I guess I could, but I think that makes more sense if the right-hand side has side effects.
Comment 3•12 years ago
|
||
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•12 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.
Comment 5•12 years ago
|
||
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•12 years ago
|
||
(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•12 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•12 years ago
|
||
Yes, see comment 5, unless you opt in, this doesn't affect your local builds...
Attachment #623480 -
Flags: feedback?(roc) → feedback+
Comment 9•12 years ago
|
||
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 10•12 years ago
|
||
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•12 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•12 years ago
|
||
Attachment #623480 -
Attachment is obsolete: true
Attachment #623953 -
Flags: review?(cpearce)
Comment 13•12 years ago
|
||
Comment on attachment 623953 [details] [diff] [review] Patch v2 Review of attachment 623953 [details] [diff] [review]: ----------------------------------------------------------------- Thanks.
Attachment #623953 -
Flags: review?(cpearce) → review+
Comment 14•12 years ago
|
||
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•12 years ago
|
||
(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 | ||
Comment 16•12 years ago
|
||
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.
Description
•