If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

fix warnings in nsRange.cpp

RESOLVED FIXED

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
4 years ago

People

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

Tracking

(Depends on: 1 bug)

Trunk
All
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 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

5 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? :)

Updated

5 years ago
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

4 years ago
Duplicate of this bug: 815065

Comment 8

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