Closed Bug 589894 Opened 14 years ago Closed 13 years ago

Fix build warnings in content/

Categories

(Core :: DOM: Core & HTML, defect, P5)

x86
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla6

People

(Reporter: Ms2ger, Assigned: Ms2ger)

Details

(Whiteboard: [build_warning])

Attachments

(1 file, 5 obsolete files)

Attached patch Patch v1 (obsolete) — Splinter Review
As noted, this is pretty low priority.
Flags: in-testsuite-
Attachment #468411 - Flags: review?(bzbarsky)
Why is the SetAttr change needed?  Is some compiler broken enough to complain about that code?
>+  nsContentType contentType = eContentTypeInherit;

For that matter, why is _this_ needed?
> +  PRUint32 stateMask = 0;

And this just makes me hate compilers.  :(
(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.
> 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.
(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*".
It could be an else, with an assert.  Does that make the warning go away?
Attached patch Patch v2 (obsolete) — Splinter Review
(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)
Attached patch Patch v3 (obsolete) — Splinter Review
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)
Attached patch Patch v4 (obsolete) — Splinter Review
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)
I don't know when I'll get a chance to get to this.... :(  Probably not before 2.0 ships.
Attached patch Patch v4.1 (obsolete) — Splinter Review
Merged to tip.
Attachment #485147 - Attachment is obsolete: true
Attachment #492049 - Flags: review?(bzbarsky)
Attachment #485147 - Flags: review?(bzbarsky)
Whiteboard: [build_warning] → [build_warning][needs review]
Blocks: 639849
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?
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)
No longer blocks: 639849
Attachment #517846 - Flags: review?(bzbarsky) → review?(Olli.Pettay)
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 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+
Looks like the recent CC changes removed that code.
Whiteboard: [build_warning][needs review] → [build_warning][needs landing]
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
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: