Last Comment Bug 589894 - Fix build warnings in content/
: Fix build warnings in content/
Status: RESOLVED FIXED
[build_warning]
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86 Linux
: P5 minor (vote)
: mozilla6
Assigned To: :Ms2ger (⌚ UTC+1/+2)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-08-23 12:53 PDT by :Ms2ger (⌚ UTC+1/+2)
Modified: 2011-04-14 13:01 PDT (History)
2 users (show)
Ms2ger: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (58.16 KB, patch)
2010-08-23 12:53 PDT, :Ms2ger (⌚ UTC+1/+2)
no flags Details | Diff | Splinter Review
Patch v2 (55.87 KB, patch)
2010-09-14 01:40 PDT, :Ms2ger (⌚ UTC+1/+2)
no flags Details | Diff | Splinter Review
Patch v3 (52.30 KB, patch)
2010-09-21 05:43 PDT, :Ms2ger (⌚ UTC+1/+2)
no flags Details | Diff | Splinter Review
Patch v4 (49.50 KB, patch)
2010-10-21 14:14 PDT, :Ms2ger (⌚ UTC+1/+2)
no flags Details | Diff | Splinter Review
Patch v4.1 (48.87 KB, patch)
2010-11-20 04:00 PST, :Ms2ger (⌚ UTC+1/+2)
no flags Details | Diff | Splinter Review
Fix a number of build warnings in content/. (42.62 KB, patch)
2011-03-08 13:01 PST, :Ms2ger (⌚ UTC+1/+2)
bugs: review+
Details | Diff | Splinter Review

Description :Ms2ger (⌚ UTC+1/+2) 2010-08-23 12:53:56 PDT
Created attachment 468411 [details] [diff] [review]
Patch v1

As noted, this is pretty low priority.
Comment 1 Boris Zbarsky [:bz] 2010-09-02 12:56:50 PDT
Why is the SetAttr change needed?  Is some compiler broken enough to complain about that code?
Comment 2 Boris Zbarsky [:bz] 2010-09-02 13:48:34 PDT
>+  nsContentType contentType = eContentTypeInherit;

For that matter, why is _this_ needed?
Comment 3 Boris Zbarsky [:bz] 2010-09-02 13:49:02 PDT
> +  PRUint32 stateMask = 0;

And this just makes me hate compilers.  :(
Comment 4 :Ms2ger (⌚ UTC+1/+2) 2010-09-03 01:01:40 PDT
(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 Boris Zbarsky [:bz] 2010-09-03 09:59:51 PDT
> 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.
Comment 6 :Ms2ger (⌚ UTC+1/+2) 2010-09-13 06:09:56 PDT
(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 Boris Zbarsky [:bz] 2010-09-13 14:57:14 PDT
It could be an else, with an assert.  Does that make the warning go away?
Comment 8 :Ms2ger (⌚ UTC+1/+2) 2010-09-14 01:40:48 PDT
Created attachment 475022 [details] [diff] [review]
Patch v2

(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.)
Comment 9 :Ms2ger (⌚ UTC+1/+2) 2010-09-21 05:43:38 PDT
Created attachment 477109 [details] [diff] [review]
Patch v3

Now without the changes in nsHTMLMediaElement.cpp, nsFrameLoader.cpp and nsDocument::Normalize.
Comment 10 :Ms2ger (⌚ UTC+1/+2) 2010-10-21 14:14:28 PDT
Created attachment 485147 [details] [diff] [review]
Patch v4

Merged to tip. Now without the stateMask initializations and the nsXMLEventsListener::InitXMLEvent hunk.
Comment 11 Boris Zbarsky [:bz] 2010-11-03 11:51:43 PDT
I don't know when I'll get a chance to get to this.... :(  Probably not before 2.0 ships.
Comment 12 :Ms2ger (⌚ UTC+1/+2) 2010-11-20 04:00:23 PST
Created attachment 492049 [details] [diff] [review]
Patch v4.1

Merged to tip.
Comment 13 :Ms2ger (⌚ UTC+1/+2) 2011-03-08 13:01:11 PST
Created attachment 517846 [details] [diff] [review]
Fix a number of build warnings in content/.
Comment 14 Boris Zbarsky [:bz] 2011-03-08 13:05:28 PST
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 15 :Ms2ger (⌚ UTC+1/+2) 2011-03-08 13:18:03 PST
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.
Comment 16 neil@parkwaycc.co.uk 2011-03-31 07:32:44 PDT
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 Olli Pettay [:smaug] 2011-03-31 09:45:32 PDT
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.
Comment 18 :Ms2ger (⌚ UTC+1/+2) 2011-04-12 13:09:47 PDT
Looks like the recent CC changes removed that code.
Comment 19 :Ms2ger (⌚ UTC+1/+2) 2011-04-14 13:01:02 PDT
http://hg.mozilla.org/mozilla-central/rev/c5f9a0fb6c67

Note You need to log in before you can comment on or make changes to this bug.