993075 - BytecodeEmitter.cpp:6825:27: warning: comparison of integers of different signs: 'unsigned int' and 'int' [-Wsign-compare]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Daniel Holbert [:dholbert]]

New build warning, introduced by Bug 981999:
{
js/src/frontend/BytecodeEmitter.cpp:6825:27: warning: comparison of integers of different signs: 'unsigned int' and 'int' [-Wsign-compare]
 1:15.53     MOZ_ASSERT(totalCount == nsrcnotes - 1);
 1:15.53                ~~~~~~~~~~ ^  ~~~~~~~~~~~~~
}

I'm hitting the warning in debug builds, using clang 3.5 (though I believe this warning is reported in all GCC & clang versions at least).

This line was added here:
 http://hg.mozilla.org/integration/mozilla-inbound/rev/4a3477a52c6f#l1.106
and we're warning because 'totalCount' is unsigned, whereas 'nsrcnotes' is signed.

Comment 1

[Daniel Holbert [:dholbert]]

Perhaps the nsrcnotes arg here (in CopySrcNotes) should be unsigned?

It's an arg for CopySrcNotes(), which was added in the cset linked in comment 0, and which only has one caller, which will always pass a non-negative value (since it range-checks its arg before calling CopySrcNotes.

It looks like that value comes from FinishTakingSrcNotes(), which also has only just started returning a signed value. Extending this logic further, maybe it should return uint32_t instead of int32_t, and just have UINT32_MAX be its sentinel failure value? (instead of forcing anything that uses its return value to have to deal with a signed number for a value that's really inherently unsigned)

Tagging till to follow up on this, since he added the code in question.

Comment 2

[Daniel Holbert [:dholbert]]

(Meant to say: I hit this in a local mozilla-inbound build. The cset that introduced this (noted in comment 0) hasn't made it to central yet.)

Comment 3

[Till Schneidereit [:till]]

Attachment: Change FinishTakingSrcNotes to return uint32_t

Sorry. To a very small extent, I blame the barrage of warnings that ICU creates. Mostly myself, obviously.

Comment 4

[Luke Wagner [:luke]]

When it's not perf-critical or a wide-spread style (like the <0 return value is in the emitter), I'd prefer to have a bool return value and a uint32_t* outparam.

Comment 5

[Till Schneidereit [:till]]

Attachment: Change FinishTakingSrcNotes to return bool and take an outparam for the source note count

Right, that's better.

Comment 6

[Luke Wagner [:luke]]

Comment on attachment 8404018 [details] [diff] [review]
Change FinishTakingSrcNotes to return bool and take an outparam for the source note count

Thanks!

Comment 7

[Till Schneidereit [:till]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c1061afe9117

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c1061afe9117