1201271 - Enable MOZ_WARN_UNUSED_RESULT for non-AppendElement{s} methods of nsTArray

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

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.

Comment 1

[Benjamin Smedberg]

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.

Comment 2

[Andrew McCreight [:mccr8]]

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.

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: Warn about unused results for more methods of nsTArray.

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.

Comment 4

[Nathan Froyd [:froydnj]]

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...

Comment 5

[Andrew McCreight [:mccr8]]

I added a comment to AnalyserNode.cpp.

Comment 6

[Pulsebot]

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

Comment 7

[Andrew McCreight [:mccr8]]

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.

Comment 8

[Andrew McCreight [:mccr8]]

Attachment: I'll land this patch rolled into the other one.

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|.

Comment 9

[Nathan Froyd [:froydnj]]

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.

Comment 10

[Nicholas Nethercote [inactive]]

Here's how I do these:

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

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

Comment 11

[Andrew McCreight [:mccr8]]

(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.

Comment 12

[Nicholas Nethercote [inactive]]

> Declaring a variable inside a macro feels a bit questionable

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

Comment 13

[Nicholas Nethercote [inactive]]

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.

Comment 14

[Pulsebot]

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

Comment 15

[Andrew McCreight [:mccr8]]

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.

Comment 16

[Andrew McCreight [:mccr8]]

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

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 18

[Andrew McCreight [:mccr8]]

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 -                    ^

Comment 19

[Pulsebot]

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

Comment 20

[Andrew McCreight [:mccr8]]

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

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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