Closed
Bug 999717
Opened 10 years ago
Closed 10 years ago
Fix Android gcc -Wlogical-op warnings
Categories
(Core :: General, defect)
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: cpeterson, Assigned: cpeterson)
References
Details
Attachments
(3 files)
1.24 KB,
patch
|
sworkman
:
review+
|
Details | Diff | Splinter Review |
1.16 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
1.03 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
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)
Assignee | ||
Comment 1•10 years ago
|
||
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)
Assignee | ||
Comment 2•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8410552 -
Flags: review?(sworkman) → review+
Updated•10 years ago
|
Attachment #8410564 -
Flags: review?(mrbkap) → review+
Updated•10 years ago
|
Attachment #8410565 -
Flags: review?(dholbert) → review+
Assignee | ||
Comment 3•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/797a9917b706 https://hg.mozilla.org/integration/mozilla-inbound/rev/5544ddbd6581 https://hg.mozilla.org/integration/mozilla-inbound/rev/891de7042cd4
status-firefox30:
--- → wontfix
status-firefox31:
--- → fixed
Comment 4•10 years ago
|
||
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
Comment 5•10 years ago
|
||
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
Comment 6•10 years ago
|
||
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")
Assignee | ||
Comment 7•10 years ago
|
||
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
Comment 8•10 years ago
|
||
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
Comment 9•10 years ago
|
||
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.)
Updated•10 years ago
|
Version: unspecified → Trunk
Comment 10•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/797a9917b706 https://hg.mozilla.org/mozilla-central/rev/891de7042cd4
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Updated•10 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 11•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/03ad79f213ba
Comment 12•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/03ad79f213ba
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•2 years ago
|
Blocks: Wlogical-op
Assignee | ||
Updated•2 years ago
|
No longer blocks: buildwarning
You need to log in
before you can comment on or make changes to this bug.
Description
•