Build warning: "nsCSSFrameConstructor.cpp:6976: warning: comparison between signed and unsigned integer expressions", from comparing nsTArray::IndexOf to kNotFound

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [build_warning])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

9 years ago
Build warning:
/mozilla/layout/base/nsCSSFrameConstructor.cpp: In function ‘void DoDeletingFrameSubtree(nsFrameManager*, nsTArray<nsIFrame*>&, nsIFrame*, nsIFrame*)’:
/mozilla/layout/base/nsCSSFrameConstructor.cpp:6976: warning: comparison between signed and unsigned integer expressions

The line it refers to is:
> 6976       NS_ASSERTION(aDestroyQueue.IndexOf(outOfFlowFrame) == kNotFound,
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#6976

This comparison is bad, because nsTArray::IndexOf returns unsigned, whereas kNotFound is -1.

kNotFound is the sentinel value for strings -- it's in nsAString.h.
nsTArray, on the other hand, should use "NoIndex" -- or better, its "Contains()" method.
(Assignee)

Comment 1

9 years ago
Created attachment 406779 [details] [diff] [review]
fix: use nsTArray::Contains
Attachment #406779 - Flags: review?(dbaron)
(Assignee)

Comment 2

9 years ago
Looks like this bug was caused by bug 474369, which switched aDestroyQueue from being a nsVoidArray (whose IndexOf returns signed values) to nsTArray (whose IndexOf returns unsigned values).
Blocks: 474369
Comment on attachment 406779 [details] [diff] [review]
fix: use nsTArray::Contains

r=dbaron
Attachment #406779 - Flags: review?(dbaron) → review+
(Assignee)

Comment 4

9 years ago
Created attachment 406784 [details] [diff] [review]
fix: use nsTArray::Contains

I did a quick MXR for usages of IndexOf in layout, to see if I could catch any more instances of this bug.

I didn't find any, but I did find two instances of comparing "IndexOf(foo)" to SomethingComplicated::NoIndex, which can be replaced with a simple "Contains" call.  This patch cleans those up, too.
Attachment #406779 - Attachment is obsolete: true
(Assignee)

Comment 5

9 years ago
Comment on attachment 406784 [details] [diff] [review]
fix: use nsTArray::Contains

dbaron -- thanks for the fast review on the first patch!

Could you take a quick glance over the two related changes in the new version?
Attachment #406784 - Flags: review?(dbaron)
Comment on attachment 406784 [details] [diff] [review]
fix: use nsTArray::Contains

r=dbaron
Attachment #406784 - Flags: review?(dbaron) → review+
(Assignee)

Comment 7

9 years ago
pushed attachment 406784 [details] [diff] [review]: http://hg.mozilla.org/mozilla-central/rev/8f9e84a49399
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Assignee)

Updated

9 years ago
Assignee: nobody → dholbert
(Assignee)

Updated

9 years ago
Whiteboard: [build_warning]
You need to log in before you can comment on or make changes to this bug.