If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in mozilla20

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years 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)

(Assignee)

Description

5 years ago
{
$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

5 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

5 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

5 years ago
Created attachment 680781 [details] [diff] [review]
fix v1

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)
(Assignee)

Comment 5

5 years ago
Created attachment 682125 [details] [diff] [review]
fix v2

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

5 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

5 years ago
Created attachment 682133 [details] [diff] [review]
fix v2a

(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+
(Assignee)

Comment 9

5 years ago
Created attachment 683838 [details] [diff] [review]
fix v3

(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

5 years ago
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+
(Assignee)

Comment 12

5 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
https://hg.mozilla.org/mozilla-central/rev/6cc2a8f21aea
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.