Closed
Bug 811057
Opened 10 years ago
Closed 10 years ago
$OBJDIR/dom/bindings/MediaStreamListBinding.cpp:77:30: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla20
People
(Reporter: dholbert, Assigned: dholbert)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 3 obsolete files)
6.17 KB,
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
{ $OBJDIR/dom/bindings/MediaStreamListBinding.cpp: In function ‘bool mozilla::dom::MediaStreamListBinding::DeferredFinalize(int32_t, void*)’: $OBJDIR//dom/bindings/MediaStreamListBinding.cpp:77:30: warning: comparison between signed and unsigned integer expressions [-Wsign-compare] } For this chunk of Codegen.py: > 804 class CGDeferredFinalize(CGAbstractStaticMethod): > 805 def __init__(self, descriptor): > 806 CGAbstractStaticMethod.__init__(self, descriptor, "DeferredFinalize", "bool", [Argument('int32_t', 'slice'), Argument('void*', 'data')]) > 807 > 808 def definition_body(self): > 809 smartPtr = DeferredFinalizeSmartPtr(self.descriptor) > 810 return """ nsTArray<%(smartPtr)s >* pointers = static_cast<nsTArray<%(smartPtr)s >*>(data); > 811 uint32_t oldLen = pointers->Length(); > 812 if (slice == -1 || slice > oldLen) { > 813 slice = oldLen; > 814 } > 815 uint32_t newLen = oldLen - slice; > 816 pointers->RemoveElementsAt(newLen, slice); https://mxr.mozilla.org/mozilla-central/source/dom/bindings/Codegen.py#804 "slice" is signed, "oldLen" is unsigned. It looks like we're basically treating slice as if it were unsigned, though, aside from the "-1" comparison. Perhaps we should be asserting that it's either -1 or nonnegative? (and then cast it to be unsigned in the comparison to |oldLen|?) And/or perhaps we should just have an "unsignedSlice" variable here which we set at line 813 and use in place of slice for the rest of the method?
Assignee | ||
Comment 1•10 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #0) > It looks like we're basically > treating slice as if it were unsigned, though, aside from the "-1" > comparison. Yeah, https://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/xpcprivate.h#733 confirms that: > 733 // Called to finalize a number of objects. Slice is the number of objects to > 734 // finalize, if it's -1 all objects should be finalized. data is the pointer "number of objects to finalize" sounds nonnegative (aside from the -1 special value)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86_64 → All
Summary: $OBJDIR//dom/bindings/MediaStreamListBinding.cpp:77:30: warning: comparison between signed and unsigned integer expressions [-Wsign-compare] → $OBJDIR/dom/bindings/MediaStreamListBinding.cpp:77:30: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
Assignee | ||
Comment 2•10 years ago
|
||
This fixes the build warning for me.
Attachment #680781 -
Flags: review?(peterv)
Comment 3•10 years ago
|
||
How about making DeferredFinalizeFunction take a uint32_t and passing in UINT32_MAX instead of -1? The check could then just be |if (slice > oldLen)|.
Comment 4•10 years ago
|
||
Or we could just do slice = NS_MIN(slice, oldLen)
Assignee | ||
Comment 5•10 years ago
|
||
OK. That's a slightly more invasive change, but it makes sense. This takes care of it, I think. (combining comment 3 & comment 4)
Attachment #680781 -
Attachment is obsolete: true
Attachment #680781 -
Flags: review?(peterv)
Attachment #682125 -
Flags: review?(peterv)
Assignee | ||
Comment 6•10 years ago
|
||
Comment on attachment 682125 [details] [diff] [review] fix v2 >diff --git a/js/xpconnect/src/XPCJSRuntime.cpp b/js/xpconnect/src/XPCJSRuntime.cpp >- int32_t counter = 0; >+ uint32_t counter = 0; [...] >- if (slice > 0 && ++counter == slice) { >+ if (slice != UINT32_MAX && ++counter == slice) { > return items->IsEmpty(); > } NOTE: This might look like it changes behavior in the situation where slice = 0 -- previously, we would've failed the first condition there ( > 0), whereas now we'll pass ( != UINT32_MAX). However, it'll still be guaranteed to fail the second condition, since |++counter| is guaranteed to evaluate to something nonzero (disregarding cases where counter wraps around from +infinity). So we'll still have the same behavior there -- we'll fail the "if" check.
Assignee | ||
Comment 7•10 years ago
|
||
(Missed one chunk in previous patch version -- s/-1/UINT32_MAX/ on a place where these DeferredFinalizeFunctions are invoked.)
Attachment #682125 -
Attachment is obsolete: true
Attachment #682125 -
Flags: review?(peterv)
Attachment #682133 -
Flags: review?(peterv)
Comment 8•10 years ago
|
||
Comment on attachment 682133 [details] [diff] [review] fix v2a Review of attachment 682133 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/src/XPCJSRuntime.cpp @@ +547,5 @@ > nsISupports *wrapper = items->ElementAt(count - 1); > items->RemoveElementAt(count - 1); > NS_RELEASE(wrapper); > > + if (slice != UINT32_MAX && ++counter == slice) { Do we still need |slice != UINT32_MAX|? If slice is UINT32_MAX and counter == slice we've dealt with all items anyway, so returning items->IsEmpty() would be the same as returning true (which we'd do currently). Unless I'm missing something. Also, it seems nonsensical to have slice == 0, so maybe we should MOZ_ASSERT that at the beginning?
Attachment #682133 -
Flags: review?(peterv) → review+
Assignee | ||
Comment 9•10 years ago
|
||
(In reply to Peter Van der Beken [:peterv] from comment #8) > Do we still need |slice != UINT32_MAX|? We don't -- you're right. I actually restructured that chunk of code to use a for-loop, up to slice (which I set to NS_MIN(slice, oldLen) along the lines of comment 4). This removes the need for the special early-return & "break" statement, making the control flow clearer IMHO. > Also, it seems nonsensical to have slice == 0, so maybe we should MOZ_ASSERT > that at the beginning? It looks like it, yeah. Added a MOZ_ASSERT to the generated function and the hardcoded "ReleaseSliceNow" impl in XPCJSRuntime. Two other minor changes: (a) Tweaked the documentation in xpcprivate.h to clarify the the meaning of the return value. (b) I caught one straggling s/-1/UINT32_MAX/, in GCCallback(). (That's the last one -- I did an audit for other mentions of "run" and "mDeferredFinalizeFunctions" in the file to be sure there weren't any other ones that I'd missed.) Requesting one more review, since I reworked the logic in ReleaseSliceNow().
Attachment #682133 -
Attachment is obsolete: true
Attachment #683838 -
Flags: review?(peterv)
Assignee | ||
Comment 10•10 years ago
|
||
Green try run (modulo a few intermittent oranges): https://tbpl.mozilla.org/?tree=Try&rev=5737da7bca87
Comment 11•10 years ago
|
||
Comment on attachment 683838 [details] [diff] [review] fix v3 Review of attachment 683838 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Thanks! ::: js/xpconnect/src/XPCJSRuntime.cpp @@ +539,5 @@ > nsTArray<nsISupports *>* items = > static_cast<nsTArray<nsISupports *>*>(data); > + > + slice = NS_MIN(slice, items->Length()); > + for (uint32_t i = 0; i < slice; i++) { ++i
Attachment #683838 -
Flags: review?(peterv) → review+
Assignee | ||
Comment 12•10 years ago
|
||
(In reply to Peter Van der Beken [:peterv] from comment #11) > > + for (uint32_t i = 0; i < slice; i++) { > > ++i Fixed, and pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/6cc2a8f21aea
Comment 13•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6cc2a8f21aea
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Updated•3 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•