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

Status: RESOLVED DUPLICATE of bug 859079

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

Description

[Daniel Holbert [:dholbert]]

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?

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(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!

Comment 2

[Daniel Holbert [:dholbert]]

(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.

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Yeah; bug 746600, but that's work :)

Comment 4

[:Cykesiopka]

Forward duping as Bug 859079 has a reviewed patch attached.