Closed
Bug 1201271
Opened 9 years ago
Closed 9 years ago
Enable MOZ_WARN_UNUSED_RESULT for non-AppendElement{s} methods of nsTArray
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: mccr8, Assigned: mccr8)
References
Details
Attachments
(2 files)
(deleted),
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
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•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.
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.
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.
Attachment #8656732 -
Flags: review?(nfroyd)
![]() |
||
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...
Attachment #8656732 -
Flags: review?(nfroyd) → review+
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.
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|.
Attachment #8657494 -
Flags: review?(nfroyd)
![]() |
||
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.
Attachment #8657494 -
Flags: review?(nfroyd) → review+
![]() |
||
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
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.
![]() |
||
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.
![]() |
||
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.
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.
Assignee | ||
Comment 16•9 years ago
|
||
I locally tested opt and debug builds of clang and gcc, so hopefully it sticks this time.
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
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 - ^
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
https://hg.mozilla.org/mozilla-central/rev/de40eee9f33f
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in
before you can comment on or make changes to this bug.
Description
•