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
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-08-23 12:53 PDT by :Ms2ger
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
no flags Details | Diff | Review
Patch v2 (55.87 KB, patch)
2010-09-14 01:40 PDT, :Ms2ger
no flags Details | Diff | Review
Patch v3 (52.30 KB, patch)
2010-09-21 05:43 PDT, :Ms2ger
no flags Details | Diff | Review
Patch v4 (49.50 KB, patch)
2010-10-21 14:14 PDT, :Ms2ger
no flags Details | Diff | Review
Patch v4.1 (48.87 KB, patch)
2010-11-20 04:00 PST, :Ms2ger
no flags Details | Diff | Review
Fix a number of build warnings in content/. (42.62 KB, patch)
2011-03-08 13:01 PST, :Ms2ger
bugs: review+
Details | Diff | Review

Description :Ms2ger 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] (Out June 25-July 6) 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] (Out June 25-July 6) 2010-09-02 13:48:34 PDT
>+  nsContentType contentType = eContentTypeInherit;

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

And this just makes me hate compilers.  :(
Comment 4 :Ms2ger 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] (Out June 25-July 6) 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 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] (Out June 25-July 6) 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 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 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 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] (Out June 25-July 6) 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 2010-11-20 04:00:23 PST
Created attachment 492049 [details] [diff] [review]
Patch v4.1

Merged to tip.
Comment 13 :Ms2ger 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] (Out June 25-July 6) 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 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] (high review load, please consider other reviewers) 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 2011-04-12 13:09:47 PDT
Looks like the recent CC changes removed that code.

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