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

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Daniel Holbert [:dholbert]]

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.

Comment 1

[Daniel Holbert [:dholbert]]

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)

Comment 2

[Daniel Holbert [:dholbert]]

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.

Comment 3

[Daniel Holbert [:dholbert]]

Botond, I think you know this code better than I do. Does comment 2 sound reasonable?

Comment 4

[Botond Ballo [:botond]]

(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).

Comment 5

[Daniel Holbert [:dholbert]]

(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.

Comment 6

[Botond Ballo [:botond]]

(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?

Comment 7

[Daniel Holbert [:dholbert]]

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.

Comment 8

[Botond Ballo [:botond]]

Attachment: Fix for the warning

Comment 9

[Botond Ballo [:botond]]

(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

Comment 10

[Daniel Holbert [:dholbert]]

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.)

Comment 11

[Botond Ballo [:botond]]

(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...

Comment 12

[Botond Ballo [:botond]]

(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

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4867ea4e9bdb