fix warnings in nsRange.cpp

RESOLVED FIXED

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: cojocaru.alex3010, Assigned: cojocaru.alex3010)

Tracking

Trunk
All
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
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]
(Assignee)

Comment 1

6 years ago
Created attachment 734360 [details] [diff] [review]
fix1
Attachment #734360 - Flags: review?(mounir)
I think we want to make mStartOffset unsigned at some point
so "uint32_t(mStartOffset)" seems like a better fix.
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.
Attachment #734360 - Flags: review?(mounir) → review+
Assignee: nobody → cojocaru.alex3010
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Hardware: x86_64 → All
Version: unspecified → Trunk
(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? :)
Depends on: 746600
> 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.
Mats or Alex, any of you can push the same patch with a cast to uint32_t instead of int32_t with r=me.

Updated

6 years ago
Duplicate of this bug: 815065

Comment 8

6 years ago
Looks like this was fixed in Bug 859193.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.