Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Fix build warnings in content/

RESOLVED FIXED in mozilla6

Status

()

Core
DOM
P5
minor
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Ms2ger, Assigned: Ms2ger)

Tracking

Trunk
mozilla6
x86
Linux
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [build_warning])

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

7 years ago
Created attachment 468411 [details] [diff] [review]
Patch v1

As noted, this is pretty low priority.
Flags: in-testsuite-
Attachment #468411 - Flags: review?(bzbarsky)

Comment 1

7 years ago
Why is the SetAttr change needed?  Is some compiler broken enough to complain about that code?

Comment 2

7 years ago
>+  nsContentType contentType = eContentTypeInherit;

For that matter, why is _this_ needed?

Comment 3

7 years ago
> +  PRUint32 stateMask = 0;

And this just makes me hate compilers.  :(
(Assignee)

Comment 4

7 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

7 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

7 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

7 years ago
It could be an else, with an assert.  Does that make the warning go away?
(Assignee)

Comment 8

7 years ago
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.)
Attachment #468411 - Attachment is obsolete: true
Attachment #475022 - Flags: review?(bzbarsky)
Attachment #468411 - Flags: review?(bzbarsky)
(Assignee)

Comment 9

7 years ago
Created attachment 477109 [details] [diff] [review]
Patch v3

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

7 years ago
Created attachment 485147 [details] [diff] [review]
Patch v4

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

7 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

7 years ago
Created attachment 492049 [details] [diff] [review]
Patch v4.1

Merged to tip.
Attachment #485147 - Attachment is obsolete: true
Attachment #492049 - Flags: review?(bzbarsky)
Attachment #485147 - Flags: review?(bzbarsky)
(Assignee)

Updated

7 years ago
Whiteboard: [build_warning] → [build_warning][needs review]
(Assignee)

Updated

7 years ago
Blocks: 639849
(Assignee)

Comment 13

7 years ago
Created attachment 517846 [details] [diff] [review]
Fix a number of build warnings in content/.
Attachment #517846 - Flags: review?(bzbarsky)

Comment 14

7 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

7 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)
(Assignee)

Updated

6 years ago
No longer blocks: 639849

Updated

6 years ago
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+
(Assignee)

Comment 18

6 years ago
Looks like the recent CC changes removed that code.
Whiteboard: [build_warning][needs review] → [build_warning][needs landing]
(Assignee)

Comment 19

6 years ago
http://hg.mozilla.org/mozilla-central/rev/c5f9a0fb6c67
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [build_warning][needs landing] → [build_warning]
Target Milestone: --- → mozilla6
You need to log in before you can comment on or make changes to this bug.