Closed Bug 815065 Opened 7 years ago Closed 7 years ago

/content/base/src/nsRange.cpp:517:48: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]

Categories

(Core :: DOM: Core & HTML, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED DUPLICATE of bug 859079

People

(Reporter: dholbert, Unassigned)

References

(Blocks 1 open bug)

Details

Two new build warnings in code introduced a few days ago in bug 804784:
{
content/base/src/nsRange.cpp: In member function ‘virtual void nsRange::CharacterDataChanged(nsIDocument*, nsIContent*, CharacterDataChangeInfo*)’:

content/base/src/nsRange.cpp:511:50: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]

content/base/src/nsRange.cpp:517:48: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
}

This is for this chunk of code:
> 510     if (parentNode == mStartParent && mStartOffset > 0 &&
> 511         mStartOffset < parentNode->GetChildCount() &&
> 512         removed == parentNode->GetChildAt(mStartOffset)) {
> 513       newStartNode = aContent;
> 514       newStartOffset = aInfo->mChangeStart;
> 515     }
> 516     if (parentNode == mEndParent && mEndOffset > 0 &&
> 517         mEndOffset < parentNode->GetChildCount() &&
https://mxr.mozilla.org/mozilla-central/source/content/base/src/nsRange.cpp#503

At lines 511 and 517, it's warning because nsINode::GetChildCount returns an unsigned value, whereas mStartOffset and mEndOffset are signed values.  Of course, we have to convert them to unsigned to do the comparison, so if they happen to be negative (as it looks like they could, from skimming elsewhere in the file), we'll get the comparison wrong.  So I think this could be a real bug in the patch.

We probably want to either cast the GetChildCount() result to be a signed integer, or we want to add a "mEndOffset < 0 ||" helper-clause to the less-than-child-count condition, I imagine?
(In reply to Daniel Holbert [:dholbert] from comment #0)
> At lines 511 and 517, it's warning because nsINode::GetChildCount returns an
> unsigned value, whereas mStartOffset and mEndOffset are signed values.  Of
> course, we have to convert them to unsigned to do the comparison, so if they
> happen to be negative (as it looks like they could, from skimming elsewhere
> in the file)

They sure shouldn't be!
(In reply to :Ms2ger from comment #1)
> (In reply to Daniel Holbert [:dholbert] from comment #0)
> > if they
> > happen to be negative (as it looks like they could, from skimming elsewhere
> > in the file)
> 
> They sure shouldn't be!

Ah, that's comforting! Maybe they should be unsigned values, then?  I didn't dig too far -- I just saw that they were set from (potentially-negative) signed integer values in DoSetRange (among other places), which has multiple callers.  Maybe those callers all do proper sign-checks, though.

In any case -- if there's any chance that these values might be negative, we should be considering that in these comparisons to unsigned values.  And otherwise, we should probably just make them unsigned.
Yeah; bug 746600, but that's work :)
Component: DOM: Traversal-Range → DOM: Core & HTML
Forward duping as Bug 859079 has a reviewed patch attached.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 859079
You need to log in before you can comment on or make changes to this bug.