APZCTreeManager.cpp:631:70: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]

RESOLVED FIXED in mozilla28

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dholbert, Assigned: botond)

Tracking

(Blocks: 1 bug)

Trunk
mozilla28
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
New build warning (on GCC 4.8, though I think other compilers warn about this too):
{
gfx/layers/composite/APZCTreeManager.cpp: In member function ‘void mozilla::layers::APZCTreeManager::HandleOverscroll(mozilla::layers::AsyncPanZoomController*, mozilla::ScreenPoint, mozilla::ScreenPoint, int)’:

gfx/layers/composite/APZCTreeManager.cpp:631:70: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]

    if (aOverscrollHandoffChainIndex >= mOverscrollHandoffChain.length()) {
                                                                       ^
}

The first half of that comparison is a signed int; the second half is an unsigned value.

Link to source:
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/APZCTreeManager.cpp?rev=8cf6021169e2&mark=631-631#623

This line was added yesterday in bug 912666.
(Reporter)

Comment 1

5 years ago
Actually, why is aOverscrollHandoffChainIndex a signed int in the first place?

shortly after the line that GCC is warning about, we have:
> 636   nsRefPtr<AsyncPanZoomController> next = mOverscrollHandoffChain[aOverscrollHandoffChainIndex];

So it had better be a nonnegative value... (in which case we might as well just make it unsigned)
(Reporter)

Comment 2

5 years ago
Yeah, at first glance, I think we want to make that argument (and the same argument in AsyncPanZoomController::AttemptScroll, which is where it comes from) an unsigned value of some flavor.
(Reporter)

Comment 3

5 years ago
Botond, I think you know this code better than I do. Does comment 2 sound reasonable?
Flags: needinfo?(botond)
(Assignee)

Comment 4

5 years ago
(In reply to Daniel Holbert [:dholbert] from comment #3)
> Botond, I think you know this code better than I do. Does comment 2 sound
> reasonable?

Yes, I'll fix that.

By the way, how do you spot these warnings? I find my build output has so many warnings that it's way too much noise to find a new one caused by code I just wrote (which I very unfortunate, because I would like to write warning-free code).
Assignee: nobody → botond
Flags: needinfo?(botond)
(Reporter)

Comment 5

5 years ago
(In reply to Botond Ballo [:botond] from comment #4)
> Yes, I'll fix that.

Thanks!
 
> By the way, how do you spot these warnings?

I just periodically check the backscroll of my build, when I'm building, and see if I see anything unfamiliar. (ignoring 3rd-party code like skia, cairo, and some media libraries)

(In reply to Botond Ballo [:botond] from comment #4)
> I find my build output has so
> many warnings that it's way too much noise to find a new one caused by code
> I just wrote

(This is precisely the reason I started trying to fix build warnings, BTW. :) )

We're actually in a pretty good state right now, warning-quantity-wise. We have 215 directories labeled as FAIL_ON_WARNINGS in their moz.build, so those are warning-free.  And if I page through the output of "./mach warnings-list", it looks like the vast majority of our warnings are in 3rd-party code like cairo, harfbuzz, speex, gtest, and gmock.
(Assignee)

Comment 6

5 years ago
(In reply to Daniel Holbert [:dholbert] from comment #5)
> (In reply to Botond Ballo [:botond] from comment #4)
> > Yes, I'll fix that.
> 
> Thanks!
>  
> > By the way, how do you spot these warnings?
> 
> I just periodically check the backscroll of my build, when I'm building, and
> see if I see anything unfamiliar. (ignoring 3rd-party code like skia, cairo,
> and some media libraries)
> 
> (In reply to Botond Ballo [:botond] from comment #4)
> > I find my build output has so
> > many warnings that it's way too much noise to find a new one caused by code
> > I just wrote
> 
> (This is precisely the reason I started trying to fix build warnings, BTW.
> :) )
> 
> We're actually in a pretty good state right now, warning-quantity-wise. We
> have 215 directories labeled as FAIL_ON_WARNINGS in their moz.build, so
> those are warning-free.  

Here is the output of my most recent build: http://people.mozilla.org/~bballo/build-output.txt
It is 15000 lines and contains over 2600 warnings. Do you really look through 15000 lines to find unfamiliar warnings among the 2600, or is your setup somehow different from mine?

> And if I page through the output of "./mach
> warnings-list", it looks like the vast majority of our warnings are in
> 3rd-party code like cairo, harfbuzz, speex, gtest, and gmock.

Is there an equivalent of "./mach warnings-list" for the b2g build system?
(Reporter)

Comment 7

5 years ago
So a huge chunk of your output seems to be from one specific "Move.h:193: warning: returning reference to temporary" (with a ton of spammy contextual information associated with each instance of that warning).  I don't get that warning, but if I did, I'd definitely file a bug to see if we can shut it down and make the build quieter. Mind filing? :)

Aside from that: no, I don't look through 15000 lines.  When I catch stuff like this, it's just because at that point in time I have a compile job running in a terminal that's visible on my workspace somewhere, and it looks like this most of the time:
 4:45.75 UnifiedBindings14.o
 4:45.95 DOMStorageObserver.o
 4:47.13 SmsSegmentInfo.o
 4:47.56 SmsServicesFactory.o
 4:47.68 UnifiedBindings15.o
(i.e. short lines, easy to ignore). And when it happens to print out something that stretches across the whole window, sometimes I'll peek over and see what it was. But if i see words like "cairo" or "skia" or another 3rd-party library on the screen, I ignore it.

Also: FWIW, ./mach warnings-list only gives me ~350 warnings right now (again, the vast majority of which are in 3rd-party libraries.) So there really aren't very many (on my machine, at least) that are actually warnings in Gecko code.

> Is there an equivalent of "./mach warnings-list" for the b2g build system?

I don't know, but presumably not if it doesn't use ./mach to build.
(Assignee)

Comment 8

5 years ago
Created attachment 8337917 [details] [diff] [review]
Fix for the warning
Attachment #8337917 - Flags: review?(dholbert)
(Assignee)

Comment 9

5 years ago
(In reply to Daniel Holbert [:dholbert] from comment #7)
> So a huge chunk of your output seems to be from one specific "Move.h:193:
> warning: returning reference to temporary" (with a ton of spammy contextual
> information associated with each instance of that warning).  I don't get
> that warning, but if I did, I'd definitely file a bug to see if we can shut
> it down and make the build quieter. Mind filing? :)

Sure: https://bugzilla.mozilla.org/show_bug.cgi?id=942980
(Reporter)

Comment 10

5 years ago
Comment on attachment 8337917 [details] [diff] [review]
Fix for the warning

Looks good to me!

(As I recall from poking into this in comment 2, the values for this parameter only come from the default argument to AttemptScroll(), which is then incremented in some recursive paths. So we don't have to worry that someone might be passing in a negative value which we're now implicitly casting to unsigned. (And even if they were, we'd already have been in trouble, I think, since per comment 1 we're using this as an index.)
Attachment #8337917 - Flags: review?(dholbert) → review+
(Assignee)

Comment 11

5 years ago
(In reply to Daniel Holbert [:dholbert] from comment #10)
> Comment on attachment 8337917 [details] [diff] [review]
> Fix for the warning
> 
> Looks good to me!

Thanks! I'll land it as soon as the trees reopen.

> (As I recall from poking into this in comment 2, the values for this
> parameter only come from the default argument to AttemptScroll(), which is
> then incremented in some recursive paths. So we don't have to worry that
> someone might be passing in a negative value which we're now implicitly
> casting to unsigned. (And even if they were, we'd already have been in
> trouble, I think, since per comment 1 we're using this as an index.)

It's nice when you can prove code correct, isn't it? :) If only it would be like that all the time...
(Assignee)

Comment 12

5 years ago
(In reply to Botond Ballo [:botond] from comment #11)
> (In reply to Daniel Holbert [:dholbert] from comment #10)
> > Comment on attachment 8337917 [details] [diff] [review]
> > Fix for the warning
> > 
> > Looks good to me!
> 
> Thanks! I'll land it as soon as the trees reopen.

https://hg.mozilla.org/integration/b2g-inbound/rev/4867ea4e9bdb
https://hg.mozilla.org/mozilla-central/rev/4867ea4e9bdb
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.