Jeff's Saturday-afternoon warning-killing rampage

RESOLVED FIXED in mozilla11

Status

()

Core
General
--
minor
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

(Blocks: 1 bug)

Trunk
mozilla11
All
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments)

While the college bowl games played and the players tried to kill each other on the field, I went and killed a bunch of Clang compiller warnings on Linux -- not in any super-orderly manner except by the type of the warning, and not necessarily comprehensively for any of them, due to hard-to-modify code like NSS, IPC, breakpad, &.
Created attachment 582631 [details] [diff] [review]
Add MOZ_FINAL to a bunch of classes with virtual methods but non-virtual destructors

Doing so silences these warnings as far as Clang's concerned (and super-recent MSVC 11, according to the bug report I filed with them).
Attachment #582631 - Flags: review?(dbaron)
Created attachment 582632 [details] [diff] [review]
Fix implicit-declaration warnings in trace-malloc

In C it's stupidly legal to declare variables without a type, and to call functions without prior declarations.  Who ever thought that was at all a good idea?  :-\
Attachment #582632 - Flags: review?(dbaron)
Created attachment 582633 [details] [diff] [review]
Remove or otherwise use a bunch of unused variables
Attachment #582633 - Flags: review?(dholbert)
Created attachment 582634 [details] [diff] [review]
Don't initialize pointers with false
Attachment #582634 - Flags: review?(dholbert)
Created attachment 582635 [details] [diff] [review]
Parenthesize assignments used directly as conditions
Attachment #582635 - Flags: review?(dholbert)
Created attachment 582636 [details] [diff] [review]
Parenthesize && expressions nested in || expressions

Some of these things were horrendously unreadable to the point where it's a wonder the original conditions were written correctly.  (Assuming they were...)
Attachment #582636 - Flags: review?(dholbert)
Comment on attachment 582633 [details] [diff] [review]
Remove or otherwise use a bunch of unused variables

>+++ b/toolkit/crashreporter/test/nsTestCrasher.cpp
>@@ -21,6 +21,8 @@ public:
> class B : A
> {
>   void f() { }
>+public:
>+  void use() { }
> };
> 
> void fcn( A* p )
>@@ -32,6 +34,7 @@ void PureVirtualCall()
> {
>   // generates a pure virtual function call
>   B b;
>+  b.use(); // make sure b's actually used

Wouldn't "(void)b;" work just as well here? I thought that was the preferred way to "use" otherwise-unused variables.  Seems simpler than adding an additional method, too.

Other than that, r=me.
Attachment #582633 - Flags: review?(dholbert) → review+

Comment 8

6 years ago
(void)b is not enough for some compilers, IIRC.
Comment on attachment 582634 [details] [diff] [review]
Don't initialize pointers with false

>--- a/js/xpconnect/src/XPCQuickStubs.cpp
>+++ b/js/xpconnect/src/XPCQuickStubs.cpp
>@@ -187,7 +187,7 @@ GeneratePropertyOp(JSContext *cx, JSObje
>     JSFunction *fun =
>         js::NewFunctionByIdWithReserved(cx, PropertyOpForwarder<Op>, argc, 0, obj, id);
>     if (!fun)
>-        return false;
>+        return NULL;

Why do your edits in this file use NULL instead of nsnull?  I assumed it was to match contextual style, but I count 30 instances of "nsnull" in this file[1], vs. only 6 instances of "NULL" [2].  Maybe the /xpconnect prefers NULL in general though? (I don't know.)

Anyway, with that mentioned, I trust your judgement, so r=me either way.

[1] http://mxr.mozilla.org/mozilla-central/search?string=nsnull&find=XPCQuickStubs.cpp&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
[2] http://mxr.mozilla.org/mozilla-central/search?string=NULL&case=on&find=XPCQuickStubs.cpp&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central (ignoring the majority of those that are *_NULL_*)
Attachment #582634 - Flags: review?(dholbert) → review+
Comment on attachment 582635 [details] [diff] [review]
Parenthesize assignments used directly as conditions

>--- a/content/base/src/nsContentSink.cpp
>+++ b/content/base/src/nsContentSink.cpp
>@@ -953,7 +953,7 @@ nsContentSink::PrefetchHref(const nsAStr
>-    if (treeItem = do_QueryInterface(docshell)) {
>+    if ((treeItem = do_QueryInterface(docshell))) {
>       treeItem->GetParent(getter_AddRefs(parentItem));
>       if (parentItem) {
>         treeItem = parentItem;

I really don't like assignment-inside-an-if-statement -- it's so easy (and so much clearer) to split it out into two consecutive lines that I don't see the point of doing it as a one-liner like this.

I'd much prefer if we split this into:
   treeItem = do_QueryInterface(docshell);
   if (treeItem) {

r=me with that (assuming you agree)
Attachment #582635 - Flags: review?(dholbert) → review+
(In reply to Josh Matthews [:jdm] from comment #8)
> (void)b is not enough for some compilers, IIRC.

Well, as another alternative, there's also "unused.h", which allows for:
  unused << b;

That also should work (& be simpler), I'd imagine.
Comment on attachment 582636 [details] [diff] [review]
Parenthesize && expressions nested in || expressions

So... this sort of warning-fix is slightly tricky, because there's always a chance that the existing logic is subtly broken -- and if that's the case, then adding parenthesis to "solidify" the existing logic would be a step in the wrong direction.

So, I looked for two things when reviewing this patch:
 (a) did you preserve the existing logic
 (b) is the (now more explicit with parens) logic sensible / what was intended

This looks good in both respects, except for one chunk where I'm suspicious about the "(b)" department.

That chunk is:
>+++ b/editor/libeditor/html/nsHTMLEditorStyle.cpp
>@@ -666,10 +666,10 @@ nsresult nsHTMLEditor::RemoveStyleInside
>   }
> 
>   // then process the node itself
>-  if ( !aChildrenOnly && 
>-        (aProperty && NodeIsType(aNode, aProperty) || // node is prop we asked for
>-        (aProperty == nsEditProperty::href && nsHTMLEditUtils::IsLink(aNode)) || // but check for link (<a href=...)
>-        (aProperty == nsEditProperty::name && nsHTMLEditUtils::IsNamedAnchor(aNode))) || // and for named anchors
>+  if ( (!aChildrenOnly &&
>+        ((aProperty && NodeIsType(aNode, aProperty)) || // node is prop we asked for
>+         (aProperty == nsEditProperty::href && nsHTMLEditUtils::IsLink(aNode)) || // but check for link (<a href=...)
>+        (aProperty == nsEditProperty::name && nsHTMLEditUtils::IsNamedAnchor(aNode)))) || // and for named anchors
>         (!aProperty && NodeIsProperty(aNode)))  // or node is any prop and we asked for that

Firstly, one whitespace nit: the second-to-last line there (for "nsEditProperty::name") needs to be indented by one more space, to stay aligned with its parenthesization-level.

But more importantly: I'm not convinced that the existing logic here is correct.

Note that the structure of the contextual code is:
  // first process the children
    [... iterate across children, making recursive calls ...]
  // then process the node itself
    [ the above-quoted conditional, which guards self-processing code ]

Given that structure, I think the "!aChildrenOnly" condition is more important than the existing logic gives it credit for -- I think it implies "process the node itself" should be skipped.  That is to say -- I suspect the logic is supposed to be:
  if ( !aChildrenOnly && (... all the other checks ...))
even if it doesn't currently behave that way.

So (unless I'm missing something) I'd prefer that we leave this particular conditional un-modified for now, and file a separate bug on investigating & correcting the logic there (named e.g. "nsHTMLEditor::RemoveStyleInside may process the passed-in node even when aChildrenOnly is set").

r=me with that conditional removed & that bug filed
Attachment #582636 - Flags: review?(dholbert) → review+
Blocks: 187528
Version: unspecified → Trunk
(In reply to Daniel Holbert [:dholbert] from comment #7)
> Wouldn't "(void)b;" work just as well here?

What jdm said.  As for unused, given that it's a test file with rather specific requirements, being that it's a breakpad test and all, I think fewer dependencies is better, so I left it using the current trick.

(In reply to Daniel Holbert [:dholbert] from comment #9)
> >--- a/js/xpconnect/src/XPCQuickStubs.cpp
> >+++ b/js/xpconnect/src/XPCQuickStubs.cpp
> Why do your edits in this file use NULL instead of nsnull?

Mistake -- code's too similar to JS engine code that uses NULL exclusively.  I can't wait until someone rewrites all the stupid nsnulls in the code base to NULL...

(In reply to Daniel Holbert [:dholbert] from comment #10)
> I really don't like assignment-inside-an-if-statement -- it's so easy (and
> so much clearer) to split it out into two consecutive lines that I don't see
> the point of doing it as a one-liner like this.

I fixed these up to not simply parenthesize -- agree that's nicer.

(In reply to Daniel Holbert [:dholbert] from comment #12)
> So (unless I'm missing something) I'd prefer that we leave this particular
> conditional un-modified for now, and file a separate bug on investigating &
> correcting the logic there (named e.g. "nsHTMLEditor::RemoveStyleInside may
> process the passed-in node even when aChildrenOnly is set").

Filed bug 711862 on this -- and I'm thinking, at least based on names, you might be right here.  (Editor code buggy?  Unpossible!)
https://hg.mozilla.org/integration/mozilla-inbound/rev/55e953f4050b
https://hg.mozilla.org/integration/mozilla-inbound/rev/fccaadf8161c
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea965a9b19f2
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4a0aafcf2d3

Still more patches to go here, don't close this bug...
https://hg.mozilla.org/mozilla-central/rev/55e953f4050b
https://hg.mozilla.org/mozilla-central/rev/fccaadf8161c
https://hg.mozilla.org/mozilla-central/rev/ea965a9b19f2
https://hg.mozilla.org/mozilla-central/rev/c4a0aafcf2d3
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
sorry, I'm distracted today :/
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla11 → ---
Comment on attachment 582631 [details] [diff] [review]
Add MOZ_FINAL to a bunch of classes with virtual methods but non-virtual destructors

r=dbaron (I'm assuming that you've tested on a compiler that gives a compilation error when there's a class derived from one with MOZ_FINAL)

Speaking of which, did you get a chance to add back the MOZ_FINAL annotations to the classes marked with NS_FINAL_CLASS that you had to not add because of (now removed) nsDerivedSafe?
Attachment #582631 - Flags: review?(dbaron) → review+
Comment on attachment 582632 [details] [diff] [review]
Fix implicit-declaration warnings in trace-malloc

If you're making an nsTypeInfo.h, could you put the declaration of nsGetTypeName in it too?  (And thus remove the redeclaration in nsTraceMalloc.c in addition to the one in nsTypeInfo.cpp.)

Also, please use PR_BEGIN_EXTERN_C etc.

r=dbaron
Attachment #582632 - Flags: review?(dbaron) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3bade82ac92
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcf9ce858829

I haven't done the final stuff yet, should do that in the bug that I opened at the time for it.

This wraps up patches here, for the m-i->m-c merge people.
Backed out on inbound because of build failures:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c62b8bcb94d4

https://tbpl.mozilla.org/php/getParsedLog.php?id=8036026&tree=Mozilla-Inbound
In file included from /builds/slave/m-in-osx64/build/xpcom/base/nsVersionComparatorImpl.cpp:38:
/builds/slave/m-in-osx64/build/xpcom/base/nsVersionComparatorImpl.h:40: error: function definition does not declare parameters
/builds/slave/m-in-osx64/build/xpcom/base/nsVersionComparatorImpl.cpp:42: error: invalid use of incomplete type 'struct nsVersionComparatorImpl'
/builds/slave/m-in-osx64/build/xpcom/base/nsVersionComparatorImpl.h:40: error: forward declaration of 'struct nsVersionComparatorImpl'
/builds/slave/m-in-osx64/build/xpcom/base/nsVersionComparatorImpl.cpp:42: error: invalid use of incomplete type 'struct nsVersionComparatorImpl'
/builds/slave/m-in-osx64/build/xpcom/base/nsVersionComparatorImpl.h:40: error: forward declaration of 'struct nsVersionComparatorImpl'
/builds/slave/m-in-osx64/build/xpcom/base/nsVersionComparatorImpl.cpp:42: error: invalid use of incomplete type 'struct nsVersionComparatorImpl'
/builds/slave/m-in-osx64/build/xpcom/base/nsVersionComparatorImpl.h:40: error: forward declaration of 'struct nsVersionComparatorImpl'
/builds/slave/m-in-osx64/build/xpcom/base/nsVersionComparatorImpl.cpp:46: error: invalid use of incomplete type 'struct nsVersionComparatorImpl'
/builds/slave/m-in-osx64/build/xpcom/base/nsVersionComparatorImpl.h:40: error: forward declaration of 'struct nsVersionComparatorImpl'

Updated

6 years ago
Duplicate of this bug: 707470
Once more, with feeling:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f9e7c73feca
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcc6c2a1da8b

I'm not sure what caused the files to which I forgot to add Attributes.h includes to compile on Linux.  I meant to take a look at .i files, then I changed my srcdir too much to be able to easily check.  :-\  Maybe I can puzzle it out of them even with further changes, and with this relanding...
https://hg.mozilla.org/mozilla-central/rev/4f9e7c73feca
https://hg.mozilla.org/mozilla-central/rev/bcc6c2a1da8b
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11

Updated

6 years ago
Duplicate of this bug: 697496
You need to log in before you can comment on or make changes to this bug.