859079 - fix warnings in nsRange.cpp

Status: RESOLVED FIXED

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

Description

[cojocaru.alex3010]

50:13.24 /home/student/mozilla-central/content/base/src/nsRange.cpp: In member function ‘virtual void nsRange::CharacterDataChanged(nsIDocument*, nsIContent*, CharacterDataChangeInfo*)’:
50:13.24 Warning: -Wsign-compare in /home/student/mozilla-central/content/base/src/nsRange.cpp: comparison between signed and unsigned integer expressions
50:13.24 /home/student/mozilla-central/content/base/src/nsRange.cpp:518:50: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
50:13.24 Warning: -Wsign-compare in /home/student/mozilla-central/content/base/src/nsRange.cpp: comparison between signed and unsigned integer expressions
50:13.24 /home/student/mozilla-central/content/base/src/nsRange.cpp:524:48: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]

Comment 1

[cojocaru.alex3010]

Attachment: fix1

Comment 2

[Mats Palmgren (inactive)]

I think we want to make mStartOffset unsigned at some point
so "uint32_t(mStartOffset)" seems like a better fix.

Comment 3

[Mounir Lamouri (:mounir)]

Comment on attachment 734360 [details] [diff] [review]
fix1

Review of attachment 734360 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with int32_t instead of int.

::: content/base/src/nsRange.cpp
@@ +514,5 @@
>      // the boundary.  (The m*Offset > 0 check is an optimization - a boundary
>      // point before the first child is never affected by normalize().)
>      nsINode* parentNode = aContent->GetParentNode();
>      if (parentNode == mStartParent && mStartOffset > 0 &&
> +        mStartOffset < (int)parentNode->GetChildCount() &&

Please use int32_t.

@@ +520,5 @@
>        newStartNode = aContent;
>        newStartOffset = aInfo->mChangeStart;
>      }
>      if (parentNode == mEndParent && mEndOffset > 0 &&
> +        mEndOffset < (int)parentNode->GetChildCount() &&

Same here.

Comment 4

[Mounir Lamouri (:mounir)]

(In reply to Mats Palmgren [:mats] from comment #2)
> I think we want to make mStartOffset unsigned at some point
> so "uint32_t(mStartOffset)" seems like a better fix.

I would be fine either way, TBH. However, I agree that using uint32_t for mStartOffset would be better. Would you mind opening a bug for this? :)

Comment 5

[Mats Palmgren (inactive)]

> Would you mind opening a bug for this? :)

It's already filed as bug 746600.

I'd still prefer uint32_t(mStartOffset) over the suggested fix, since when
someone fixes 746600 they can *blindly* change "uint32_t(mStartOffset)" ->
"mStartOffset" without changing the meaning of the code.  OTOH, every use
of int32_t have to be carefully reviewed, so we shouldn't add more of that.
Not a big deal, but I thought I should explain my motivation.

Comment 6

[Mounir Lamouri (:mounir)]

Mats or Alex, any of you can push the same patch with a cast to uint32_t instead of int32_t with r=me.

Comment 8

[:Cykesiopka]

Looks like this was fixed in Bug 859193.