Closed Bug 993075 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: dholbert, Assigned: till)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.
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.
Flags: needinfo?(till)
(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.)
Sorry. To a very small extent, I blame the barrage of warnings that ICU creates. Mostly myself, obviously.
Attachment #8403904 - Flags: review?(luke)
Assignee: nobody → till
Status: NEW → ASSIGNED
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.
Attachment #8403904 - Attachment is obsolete: true
Attachment #8403904 - Flags: review?(luke)
Comment on attachment 8404018 [details] [diff] [review]
Change FinishTakingSrcNotes to return bool and take an outparam for the source note count

Thanks!
Attachment #8404018 - Flags: review?(luke) → review+
https://hg.mozilla.org/mozilla-central/rev/c1061afe9117
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.