Last Comment Bug 750356 - ReusableTileStoreOGL.cpp:16:37: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
: ReusableTileStoreOGL.cpp:16:37: warning: comparison between signed and unsign...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics: Layers (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla15
Assigned To: Daniel Holbert [:dholbert]
:
: Milan Sreckovic [:milan]
Mentors:
Depends on:
Blocks: buildwarning 745177
  Show dependency treegraph
 
Reported: 2012-04-30 10:57 PDT by Daniel Holbert [:dholbert]
Modified: 2012-05-02 21:18 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix v1 (1.67 KB, patch)
2012-04-30 11:11 PDT, Daniel Holbert [:dholbert]
chrislord.net: review+
Details | Diff | Splinter Review

Description Daniel Holbert [:dholbert] 2012-04-30 10:57:00 PDT
Build warnings:

../../../mozilla/gfx/layers/opengl/ReusableTileStoreOGL.cpp: In destructor ‘mozilla::layers::ReusableTileStoreOGL::~ReusableTileStoreOGL()’:
../../../mozilla/gfx/layers/opengl/ReusableTileStoreOGL.cpp:16:37: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
../../../mozilla/gfx/layers/opengl/ReusableTileStoreOGL.cpp: In member function ‘void mozilla::layers::ReusableTileStoreOGL::HarvestTiles(mozilla::layers::TiledLayerBufferOGL*, const nsIntSize&, const nsIntRegion&, const nsIntRegion&, const gfxSize&, const gfxSize&)’:
../../../mozilla/gfx/layers/opengl/ReusableTileStoreOGL.cpp:39:37: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]


This is from loops like
> for (int i = 0; i < mTiles.Length();)
where mTiles.Length() is unsigned.
Comment 1 Daniel Holbert [:dholbert] 2012-04-30 11:11:16 PDT
Created attachment 619617 [details] [diff] [review]
fix v1
Comment 2 Chris Lord [:cwiiis] 2012-04-30 12:32:58 PDT
Comment on attachment 619617 [details] [diff] [review]
fix v1

Review of attachment 619617 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine, and my apologies for letting this in - Instead of using PRUInt32, it might be nice to use size_t, as that's what's used elsewhere in the file/gfx (and what nsTArray returns, I think?)
Comment 3 Daniel Holbert [:dholbert] 2012-04-30 12:41:27 PDT
(In reply to Chris Lord [:cwiiis] from comment #2)
> Looks fine, and my apologies for letting this in - Instead of using
> PRUInt32, it might be nice to use size_t, as that's what's used elsewhere in
> the file/gfx (and what nsTArray returns, I think?)

Nope, nsTArray uses PRUint32. Internally it calls it ::size_type / ::index_type:
  http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray.h#219

So we should technically be using...
 nsTArray< nsAutoPtr<ReusableTiledTextureOGL> >::size_type 
or
 nsTArray< nsAutoPtr<ReusableTiledTextureOGL> >::index_type

...but that's super-long, so I just used what it was typedef'd to.

But if you prefer size_t, I can switch it.
Comment 4 Chris Lord [:cwiiis] 2012-04-30 15:45:37 PDT
(In reply to Daniel Holbert [:dholbert] from comment #3)
> (In reply to Chris Lord [:cwiiis] from comment #2)
> > Looks fine, and my apologies for letting this in - Instead of using
> > PRUInt32, it might be nice to use size_t, as that's what's used elsewhere in
> > the file/gfx (and what nsTArray returns, I think?)
> 
> Nope, nsTArray uses PRUint32. Internally it calls it ::size_type /
> ::index_type:
>   http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray.h#219
> 
> So we should technically be using...
>  nsTArray< nsAutoPtr<ReusableTiledTextureOGL> >::size_type 
> or
>  nsTArray< nsAutoPtr<ReusableTiledTextureOGL> >::index_type
> 
> ...but that's super-long, so I just used what it was typedef'd to.
> 
> But if you prefer size_t, I can switch it.

Nope, PRUint32 is good with me, I was under a false impression - if there are any cases of size_t elsewhere in the file, feel free to change those too.
Comment 5 Daniel Holbert [:dholbert] 2012-04-30 15:51:15 PDT
Cool -- I added one s/size_t/PRUint32/ tweak, for consistency, and pushed:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/69b13912439f

Thanks!

Note You need to log in before you can comment on or make changes to this bug.