Closed
Bug 589894
Opened 14 years ago
Closed 13 years ago
Fix build warnings in content/
Categories
(Core :: DOM: Core & HTML, defect, P5)
Tracking
()
RESOLVED
FIXED
mozilla6
People
(Reporter: Ms2ger, Assigned: Ms2ger)
Details
(Whiteboard: [build_warning])
Attachments
(1 file, 5 obsolete files)
42.62 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
As noted, this is pretty low priority.
Flags: in-testsuite-
Attachment #468411 -
Flags: review?(bzbarsky)
Comment 1•14 years ago
|
||
Why is the SetAttr change needed? Is some compiler broken enough to complain about that code?
Comment 2•14 years ago
|
||
>+ nsContentType contentType = eContentTypeInherit;
For that matter, why is _this_ needed?
Comment 3•14 years ago
|
||
> + PRUint32 stateMask = 0;
And this just makes me hate compilers. :(
Assignee | ||
Comment 4•14 years ago
|
||
(In reply to comment #1) > Why is the SetAttr change needed? Is some compiler broken enough to complain > about that code? GCC 4.2: /Mozilla/mozilla-central/content/base/src/nsGenericElement.cpp: In member function ‘virtual nsresult nsGenericElement::SetAttr(PRInt32, nsIAtom*, nsIAtom*, const nsAString_internal&, PRBool)’: /Mozilla/mozilla-central/content/base/src/nsGenericElement.cpp:4581: warning: ‘valueMatches’ may be used uninitialized in this function Why is that an else if rather than an if, though? (In reply to comment #2) > >+ nsContentType contentType = eContentTypeInherit; > > For that matter, why is _this_ needed? No idea. (In reply to comment #3) > > + PRUint32 stateMask = 0; > > And this just makes me hate compilers. :( You didn't before? Not much to do about it, I'm afraid.
Comment 5•14 years ago
|
||
> GCC 4.2: Worksforme over here, with gcc 4.2 on mac... I'd really not add more code to this codepath if we can avoid it; it's kinda perf-sensitive. And in this case, the analysis that valueMatches is always set should be trivial for the compiler. I generally object to working around this known gcc bug. > Why is that an else if rather than an if, though? Because if both are true we don't want to do the work twice. > No idea. OK. It's not a huge deal unlike the SetAttr thing, since this isn't perf-sensitive, but it's weird to have to add it, since all codepaths obviously init it.
Assignee | ||
Comment 6•14 years ago
|
||
(In reply to comment #5) > > Why is that an else if rather than an if, though? > > Because if both are true we don't want to do the work twice. Of course. What I actually meant to ask is "Why is that an else if rather than an *else*".
Comment 7•14 years ago
|
||
It could be an else, with an assert. Does that make the warning go away?
Assignee | ||
Comment 8•14 years ago
|
||
(In reply to comment #7) > It could be an else, with an assert. Does that make the warning go away? It does. (The hunks in content/canvas/src/CustomQS_WebGL.h and content/smil/nsSMILTimeValueSpec.cpp didn't apply anymore.)
Attachment #468411 -
Attachment is obsolete: true
Attachment #475022 -
Flags: review?(bzbarsky)
Attachment #468411 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 9•14 years ago
|
||
Now without the changes in nsHTMLMediaElement.cpp, nsFrameLoader.cpp and nsDocument::Normalize.
Attachment #475022 -
Attachment is obsolete: true
Attachment #477109 -
Flags: review?(bzbarsky)
Attachment #475022 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 10•14 years ago
|
||
Merged to tip. Now without the stateMask initializations and the nsXMLEventsListener::InitXMLEvent hunk.
Attachment #477109 -
Attachment is obsolete: true
Attachment #485147 -
Flags: review?(bzbarsky)
Attachment #477109 -
Flags: review?(bzbarsky)
Comment 11•14 years ago
|
||
I don't know when I'll get a chance to get to this.... :( Probably not before 2.0 ships.
Assignee | ||
Comment 12•14 years ago
|
||
Merged to tip.
Attachment #485147 -
Attachment is obsolete: true
Attachment #492049 -
Flags: review?(bzbarsky)
Attachment #485147 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•13 years ago
|
Whiteboard: [build_warning] → [build_warning][needs review]
Assignee | ||
Comment 13•13 years ago
|
||
Attachment #517846 -
Flags: review?(bzbarsky)
Comment 14•13 years ago
|
||
This is a very low priority review for me at the moment.... Please pick on someone else if you want this soon? Also, do both patches actually need review? Or just the second one?
Assignee | ||
Comment 15•13 years ago
|
||
Comment on attachment 492049 [details] [diff] [review] Patch v4.1 Just the latter. I'll just put bug 639849 below this one in my queue when I next rebase.
Attachment #492049 -
Attachment is obsolete: true
Attachment #492049 -
Flags: review?(bzbarsky)
Updated•13 years ago
|
Attachment #517846 -
Flags: review?(bzbarsky) → review?(Olli.Pettay)
Comment 16•13 years ago
|
||
Comment on attachment 517846 [details] [diff] [review] Fix a number of build warnings in content/. >+ bool inserted = !! > #endif >- >- PRBool inserted = >- !!(sWSsConnecting->InsertElementSorted(this, nsWSNetAddressComparator())); >+ sWSsConnecting->InsertElementSorted(this, nsWSNetAddressComparator()); > NS_ASSERTION(inserted, "Couldn't insert the ws connection into the " > "serialization list."); Whoa, PRBool was broken (could assert erroneously on 64-bit OS)! Possibly void* would have worked. Issue with !! is that this is debug code so the compiler doesn't get to optimise it away.
Comment 17•13 years ago
|
||
Comment on attachment 517846 [details] [diff] [review] Fix a number of build warnings in content/. I don't particularly like the random PRBool -> bool changes, but no need to change those. >diff --git a/content/xul/document/src/nsXULDocument.cpp b/content/xul/document/src/nsXULDocument.cpp >--- a/content/xul/document/src/nsXULDocument.cpp >+++ b/content/xul/document/src/nsXULDocument.cpp >@@ -383,19 +383,16 @@ NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_ > NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mLocalStore) > > if (tmp->mOverlayLoadObservers.IsInitialized()) > tmp->mOverlayLoadObservers.EnumerateRead(TraverseObservers, &cb); > if (tmp->mPendingOverlayLoadNotifications.IsInitialized()) > tmp->mPendingOverlayLoadNotifications.EnumerateRead(TraverseObservers, &cb); > NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END > >-NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(nsXULDocument, nsXMLDocument) >-NS_IMPL_CYCLE_COLLECTION_UNLINK_END > >- NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(nsXULDocument, nsXMLDocument) >+ NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED_NO_UNLINK(nsXULDocument, >+ nsXMLDocument) > Does this really create some warning? We'll need to add things to the unlink, so would be better to not remove it now.
Attachment #517846 -
Flags: review?(Olli.Pettay) → review+
Assignee | ||
Comment 18•13 years ago
|
||
Looks like the recent CC changes removed that code.
Whiteboard: [build_warning][needs review] → [build_warning][needs landing]
Assignee | ||
Comment 19•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/c5f9a0fb6c67
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [build_warning][needs landing] → [build_warning]
Target Milestone: --- → mozilla6
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•