Closed Bug 999717 Opened 6 years ago Closed 6 years ago

Fix Android gcc -Wlogical-op warnings

Categories

(Core :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31
Tracking Status
firefox30 --- wontfix
firefox31 --- fixed

People

(Reporter: cpeterson, Assigned: cpeterson)

References

(Blocks 1 open bug)

Details

Attachments

(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 https://hg.mozilla.org/integration/mozilla-inbound/rev/7f1464f22982
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...
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleAnimation.cpp?rev=32f48d6d3389&mark=3258-3260,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"
https://bugzilla.mozilla.org/attachment.cgi?id=796597&action=diff#a/layout/style/nsStyleAnimation.cpp_sec11

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

https://hg.mozilla.org/mozilla-central/annotate/ac376a4e8174/layout/style/nsStyleAnimation.h#l224
Good, all is well then.

rs=me on restoring this assertion to its pre-bug 896050 state, i.e. restoring the deleted lines here:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/4eee1c589bb0#l2.489
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
https://hg.mozilla.org/integration/mozilla-inbound/diff/f2b02ba56bdd/layout/style/nsStyleAnimation.cpp#l1.982
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
https://hg.mozilla.org/mozilla-central/rev/797a9917b706
https://hg.mozilla.org/mozilla-central/rev/891de7042cd4
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/03ad79f213ba
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.