$OBJDIR/dom/bindings/MediaStreamListBinding.cpp:77:30: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]

RESOLVED FIXED in mozilla20

Status

()

defect
RESOLVED FIXED
7 years ago
5 months ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

(Blocks 1 bug)

Trunk
mozilla20
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

{
$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?
(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: 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]
Posted patch fix v1 (obsolete) — Splinter Review
This fixes the build warning for me.
Attachment #680781 - Flags: review?(peterv)
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)|.
Or we could just do slice = NS_MIN(slice, oldLen)
Posted patch fix v2 (obsolete) — Splinter Review
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)
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.
Posted patch fix v2a (obsolete) — Splinter Review
(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 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+
Posted patch fix v3Splinter Review
(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)
Green try run (modulo a few intermittent oranges):
 https://tbpl.mozilla.org/?tree=Try&rev=5737da7bca87
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+
(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
https://hg.mozilla.org/mozilla-central/rev/6cc2a8f21aea
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.