Closed Bug 999717 Opened 7 years ago Closed 7 years ago

Fix Android gcc -Wlogical-op warnings


(Core :: General, defect)

Not set



Tracking Status
firefox30 --- wontfix
firefox31 --- fixed


(Reporter: cpeterson, Assigned: cpeterson)


(Blocks 1 open bug)



(3 files)

netwerk/base/src/nsSocketTransport2.cpp:2436:87 [-Wlogical-op] logical 'or' of collectively exhaustive tests is always true
netwerk/base/src/nsSocketTransport2.cpp:2438:97 [-Wlogical-op] logical 'or' of collectively exhaustive tests is always true
netwerk/base/src/nsSocketTransport2.cpp:2440:89 [-Wlogical-op] logical 'or' of collectively exhaustive tests is always true
Attachment #8410552 - Flags: review?(sworkman)
Fix gcc -Wlogical-op warning in js:

js/xpconnect/src/XPCConvert.cpp:142:35 [-Wlogical-op] logical 'or' of collectively exhaustive tests is always true

Now that XPConnect uses C++ bool instead of PRBool, the malformed PRBool assertions from bug 658351 should no longer be necessary.
Attachment #8410564 - Flags: review?(mrbkap)
Fix gcc -Wlogical-op warning in layout:

layout/style/nsStyleAnimation.cpp:3734:51 [-Wlogical-op] logical 'or' of collectively exhaustive tests is always true
Attachment #8410565 - Flags: review?(dholbert)
Attachment #8410552 - Flags: review?(sworkman) → review+
Attachment #8410564 - Flags: review?(mrbkap) → review+
Attachment #8410565 - Flags: review?(dholbert) → review+
ABORT: dasharrays and filters may not be null: '(aUnit != eUnit_Dasharray && aUnit != eUnit_Filter) || aValueList != nullptr', file /builds/slave/m-in-osx64-d-00000000000000000/build/layout/style/nsStyleAnimation.cpp, line 3736

mochitest-3, -5, -devtools3, crashtests.

Backed out the layout patch in
If I place a breakpoint on the assertion, I can confirm that we do indeed hit this line with aUnit ==  eUnit_Filter and aValueList == nullptr.
Blocks: 896050
And that's because we're hitting this code...,3306-3307#3255
...with filters.Length() == 0, so we never set 'result' to anything non-null before the SetAndAdoptCSSValueListValue() call. This happens when we're generating a computed value for animating from "filter:none", and it seems normal that this would generate a 0-length nsTArray of filters.

Moreover, I'm noticing that the assertion right before this in the patch that last touched this assertion says: "value lists other than shadows and filters may not be null"

...which increases my confidence that it's OK for us to have null here.

So I think the assertion in attachment 8410565 [details] [diff] [review] here could probably be reverted to only care about dasharray, as it did before bug 896050.  Though possibly it should even be expanded to be closer to the assertion linked above. (about "value lists other than shadows and filters")
The eUnit_Filter enum definition has a comment saying "nsCSSValueList* (may be null)", so the assertion was wrong:
Good, all is well then.

rs=me on restoring this assertion to its pre-bug 896050 state, i.e. restoring the deleted lines here:
or better [with a bit more sanity-checking], rs=me on tweaking the assertion to match the other one in operator= that I linked to in comment 6, per IRC.

(The comments next to the enum definitions that you linked to in comment 7 support this.  Also, code archeology shows that that assertion inside of operator= was broadened from "dasharrays may not be null" to "value lists other than shadows may not be null" in
and we probably should have made an equivalent change to the assertion in SetAndAdoptCSSValueListValue() in that commit. That's why they weren't consistent when bug 896050 added filter support. This will make them consistent again, which is good.)
Version: unspecified → Trunk
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Resolution: FIXED → ---
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.