Andrew McCreight [:mccr8] Assignee
	Description • 9 years ago

There are a huge number of places that don't check the result of AppendElement and related methods, but I think we could enable warning for results for other methods without too much fuss.

	Benjamin Smedberg
	Comment 1 • 9 years ago

Is this for the fallible or infallible forms? The fallible forms should definitely be WARN_UNUSED, but hopefully most people are using the infallible form.

	Andrew McCreight [:mccr8] Assignee
	Comment 2 • 9 years ago

There are a bunch of commented out MOZ_WARN_UNUSED_RESULT from bug 968520. It looks like they are all fallible methods. My patch just uncomments them.

	Andrew McCreight [:mccr8] Assignee
	Comment 3 • 9 years ago

This leaves alone the AppendElement methods, but adds the annotations for the rest. I removed some (void) annotations that I think are not needed any more because the infallible variants have no return type. These were found by manual inspection.

In a few places, I had to change (void)foo() to unused << foo() because GCC does not accept the former as a way to ignore this warning, only Clang does. I also had to add it to the array testing file.

This is green on Linux32, Win32 and OSX:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a7451e88132
Hopefully that's enough coverage.

	Nathan Froyd [:froydnj]
	Comment 4 • 9 years ago

Comment on attachment 8656732 [details] [diff] [review]
Warn about unused results for more methods of nsTArray.

Review of attachment 8656732 [details] [diff] [review]:
-----------------------------------------------------------------

r=me assuming this compiles everywhere.

::: dom/media/webaudio/AnalyserNode.cpp
@@ +91,5 @@
>  
>    // Enough chunks must be recorded to handle the case of fftSize being
>    // increased to maximum immediately before getFloatTimeDomainData() is
>    // called, for example.
> +  unused << mChunks.SetLength(CHUNK_COUNT, fallible);

This is weird code, but appears to be correct after looking through AnalyserNode.cpp--all accesses to mChunks are zero-length-checked.  This bit of code could use a comment describing that...maybe as a followup?

::: xpcom/tests/TestTArray.cpp
@@ +1115,2 @@
>    // Setup test arrays.
> +  FOR_EACH(; unused << , .SetLength(N, fallible));

This is kind of weird, but I suppose it is not much worse than the current state of affairs...

	Andrew McCreight [:mccr8] Assignee
	Comment 5 • 9 years ago

I added a comment to AnalyserNode.cpp.

	Pulsebot
	Comment 6 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/351d5f864f9e

	Andrew McCreight [:mccr8] Assignee
	Comment 7 • 9 years ago

I backed this out, though I guess I didn't use the right verbiage to get it to show up here.
https://hg.mozilla.org/integration/mozilla-inbound/rev/10208177e0fe

Anyways, while I did an all-platform push, I did debug-only and not opt, and it turns out MOZ_ALWAYS_TRUE and _FALSE use (void) to silence warnings in opt builds which as I said above doesn't actually work in GCC. Also, sometimes MOZ_ALWAYS_* is not used inside a mozilla namespace, so you have to do mozilla::unused <<. Also, sometimes Assertions.h is included in a C file, so you sometimes have to use (void). With all that done, I can locally do an opt build with GCC, but I'll do a for-realz all-platform build to double check.

	Andrew McCreight [:mccr8] Assignee
	Comment 8 • 9 years ago

all platform, debug and opt try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=956f012eb353

MOZ_ALWAYS_{TRUE,FALSE} should silence MOZ_WARN_UNUSED_RESULT.

MOZ_ALWAYS_TRUE and MOZ_ALWAYS_FALSE are used for the results of
nsTArray methods that are being given MOZ_WARN_UNUSED_RESULT. They
currently use (void) to silence compiler warnings, but this is
insufficient for gcc. Instead, we need to use |unused <<| to silence
the warning, which in turn requires including mozilla/unused.h in
Assertions.h. This #include is only needed for non-debug builds, but
because of the large number of build errors this #include caused (see
below), I decided it would be better to always include it. This
include causes a number of problems.

The first problem is that Assertions.h is included in some C files, so
we must guard it to not include unused.h (and fall back on the old
definition of MOZ_ALWAYS_*) for C files.

The second problem is that some files use MOZ_ALWAYS_* outside of the
mozilla namespace, so their definitions must explicitly use
mozilla::unused.

The third problem is that there are some files that include
Assertions.h that define local variables named |unused|, which causes
shadowing errors in Clang. This is why I changed nsXMLPrettyPrinter
and nsRuleNode.

The fourth problem is that at least one file
(GeckoChildProcessHost.cpp), includes both Assertions.h and Chromium
IPC logging. The former now includes unused.h, so there are two
definitions of operator<<. Somehow operator overload resolution
decided the IPC logging must be using the |unused| one, which fails to
typecheck. My solution to this was to make the |unused| operator<< a
method on |unused|.

	Nathan Froyd [:froydnj]
	Comment 9 • 9 years ago

Comment on attachment 8657494 [details] [diff] [review]
I'll land this patch rolled into the other one.

Review of attachment 8657494 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the MOZ_ALWAYS_TRUE (resp. MOZ_ALWAYS_FALSE) change below, assuming it works.

::: mfbt/Assertions.h
@@ +496,5 @@
>  #ifdef DEBUG
>  #  define MOZ_ALWAYS_TRUE(expr)      MOZ_ASSERT((expr))
>  #  define MOZ_ALWAYS_FALSE(expr)     MOZ_ASSERT(!(expr))
> +#elif defined(__cplusplus)
> +// gcc doesn't consider (void) sufficient to silence MOZ_WARN_UNUSED_RESULT.

Man, I found the GCC PRs on this and they are depressing.

@@ +498,5 @@
>  #  define MOZ_ALWAYS_FALSE(expr)     MOZ_ASSERT(!(expr))
> +#elif defined(__cplusplus)
> +// gcc doesn't consider (void) sufficient to silence MOZ_WARN_UNUSED_RESULT.
> +#  define MOZ_ALWAYS_TRUE(expr)      (mozilla::unused << (expr))
> +#  define MOZ_ALWAYS_FALSE(expr)     (mozilla::unused << (expr))

Here's an alternative suggestion:

#  define MOZ_ALWAYS_TRUE(expr) \
  do { \
    if (expr) { \
      /* Definitively silence MOZ_WARN_UNUSED_RESULT. */ \
    } \
  } while(0)

We avoid a lot of churn that way and also avoid adding mozilla::unused in weird places.

	Nicholas Nethercote [inactive]
	Comment 10 • 9 years ago

Here's how I do these:

> int dummy = foo();
> (void) dummy;

E.g. https://hg.mozilla.org/mozilla-central/rev/c208702ec139

	Andrew McCreight [:mccr8] Assignee
	Comment 11 • 9 years ago

(In reply to Nicholas Nethercote [:njn] from comment #10)
> > int dummy = foo();
> > (void) dummy;

Declaring a variable inside a macro feels a bit questionable, though maybe it would work. I'll try Nathan's idea.

	Nicholas Nethercote [inactive]
	Comment 12 • 9 years ago

> Declaring a variable inside a macro feels a bit questionable

I think it's fine if you block-scope it.

	Nicholas Nethercote [inactive]
	Comment 13 • 9 years ago

I just filed bug 1202965 for adding MOZ_UNUSED as an alternative to mozilla::unused, which is relevant to this bug but may not end up causing its patches to be changed.

	Pulsebot
	Comment 14 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/f5b2a11c64c9

	Andrew McCreight [:mccr8] Assignee
	Comment 15 • 9 years ago

I went with Nathan's suggestion in comment 9. Thanks, that was much simpler. I'll file a followup bug for changing operator<< for unused, as I think it is a simple enough change and it might help somebody in the future.

	Andrew McCreight [:mccr8] Assignee
	Comment 16 • 9 years ago

I locally tested opt and debug builds of clang and gcc, so hopefully it sticks this time.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 17 • 9 years ago

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/f85819521302 for android bustage:

https://treeherder.mozilla.org/logviewer.html#?job_id=13844748&repo=mozilla-inbound

	Andrew McCreight [:mccr8] Assignee
	Comment 18 • 9 years ago

Well, using |if| broke Android pretty hard, with errors like this a bunch of times:

15:50:24     INFO -  /builds/slave/m-in-and-api-9-000000000000000/build/src/obj-firefox/widget/android/bindings/Bundle.cpp:56:67:   required from here
15:50:24     INFO -  ../../../dist/include/mozilla/jni/Accessors.h:103:18: error: suggest parentheses around assignment used as truth value [-Werror=parentheses]
15:50:24     INFO -               MOZ_ALWAYS_TRUE(sID = AndroidBridge::GetStaticMethodID(
15:50:24     INFO -                    ^

	Pulsebot
	Comment 19 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/de40eee9f33f

	Andrew McCreight [:mccr8] Assignee
	Comment 20 • 9 years ago

I added some parens and did a for-real all-platform try build run last week. Hopefully nothing more has broken in the interim.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=81786870434f

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 21 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/de40eee9f33f