[ignition] Fix more -Wsign-compare warnings.

[ignition] Fix more -Wsign-compare warnings.

BUG=v8:5614
Committed: https://crrev.com/2bd1ee96a555ae28d1efeb018d725fb577ae35d8
Cr-Commit-Position: refs/heads/master@{#40918}

[ulan, 2016-11-11 10:51:29]

ulan@chromium.org changed reviewers:
+ rmcilroy@chromium.org

[ulan, 2016-11-11 10:51:30]

ptal, after this ignition compiles without warnings both in release and debug
modes.

[ulan, 2016-11-11 11:40:29]

ulan@chromium.org changed reviewers:
+ mstarzinger@chromium.org

[ulan, 2016-11-11 11:40:30]

Redirecting to Michael.

[Michael Starzinger, 2016-11-11 11:45:43]

LGTM. Thanks!

https://codereview.chromium.org/2493183002/diff/1/test/unittests/interpreter/...
File test/unittests/interpreter/constant-array-builder-unittest.cc (right):

https://codereview.chromium.org/2493183002/diff/1/test/unittests/interpreter/...
test/unittests/interpreter/constant-array-builder-unittest.cc:69:
CHECK_EQ(constant_array->length(), kNumberOfElements);
nit: As discussed offline: If the bogus conversion is this one, then I would
vote for keeping the variables size_t and do a cast here instead.

[ulan, 2016-11-11 11:47:26]

Thank you.

https://codereview.chromium.org/2493183002/diff/1/test/unittests/interpreter/...
File test/unittests/interpreter/constant-array-builder-unittest.cc (right):

https://codereview.chromium.org/2493183002/diff/1/test/unittests/interpreter/...
test/unittests/interpreter/constant-array-builder-unittest.cc:69:
CHECK_EQ(constant_array->length(), kNumberOfElements);
On 2016/11/11 11:45:43, Michael Starzinger wrote:
> nit: As discussed offline: If the bogus conversion is this one, then I would
> vote for keeping the variables size_t and do a cast here instead.

Done.

[ulan, 2016-11-11 11:47:45]

The CQ bit was checked by ulan@chromium.org

[ulan, 2016-11-11 11:47:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/2493183002/#ps20001
(title: "Address comment")

[commit-bot: I haz the power, 2016-11-11 11:47:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rmcilroy, 2016-11-11 11:48:52]

LGTM, thanks.

https://codereview.chromium.org/2493183002/diff/1/test/unittests/interpreter/...
File test/unittests/interpreter/constant-array-builder-unittest.cc (right):

https://codereview.chromium.org/2493183002/diff/1/test/unittests/interpreter/...
test/unittests/interpreter/constant-array-builder-unittest.cc:69:
CHECK_EQ(constant_array->length(), kNumberOfElements);
On 2016/11/11 11:45:43, Michael Starzinger wrote:
> nit: As discussed offline: If the bogus conversion is this one, then I would
> vote for keeping the variables size_t and do a cast here instead.

Any reason to do this? It seems like Ulan's approach drops a bunch of unecessary
static_casts and doesn't have much negatives AFAIKT.

[ulan, 2016-11-11 11:52:44]

On 2016/11/11 11:48:52, rmcilroy wrote:
> LGTM, thanks.
> 
>
https://codereview.chromium.org/2493183002/diff/1/test/unittests/interpreter/...
> File test/unittests/interpreter/constant-array-builder-unittest.cc (right):
> 
>
https://codereview.chromium.org/2493183002/diff/1/test/unittests/interpreter/...
> test/unittests/interpreter/constant-array-builder-unittest.cc:69:
> CHECK_EQ(constant_array->length(), kNumberOfElements);
> On 2016/11/11 11:45:43, Michael Starzinger wrote:
> > nit: As discussed offline: If the bogus conversion is this one, then I would
> > vote for keeping the variables size_t and do a cast here instead.
> 
> Any reason to do this? It seems like Ulan's approach drops a bunch of
unecessary
> static_casts and doesn't have much negatives AFAIKT.

As Michael explained to me, eventually this code will use size_t. So it ensures
forward progress.

[Michael Starzinger, 2016-11-11 11:54:33]

https://codereview.chromium.org/2493183002/diff/1/test/unittests/interpreter/...
File test/unittests/interpreter/constant-array-builder-unittest.cc (right):

https://codereview.chromium.org/2493183002/diff/1/test/unittests/interpreter/...
test/unittests/interpreter/constant-array-builder-unittest.cc:69:
CHECK_EQ(constant_array->length(), kNumberOfElements);
On 2016/11/11 11:48:52, rmcilroy wrote:
> On 2016/11/11 11:45:43, Michael Starzinger wrote:
> > nit: As discussed offline: If the bogus conversion is this one, then I would
> > vote for keeping the variables size_t and do a cast here instead.
> 
> Any reason to do this? It seems like Ulan's approach drops a bunch of
unecessary
> static_casts and doesn't have much negatives AFAIKT.

My reasoning was that safe indexed vector accessors (like in [1]) should operate
on unsigned values. Similar container sizes are usually represented with
unsigned values (like in [2]). Maybe there are other factors at play in
{ConstantArrayBuilder} that I am not aware of, in which case I am sorry for the
distraction. Also, since this is test code, I don't care all too much.

[1] http://en.cppreference.com/w/cpp/container/vector/at
[2] http://en.cppreference.com/w/cpp/container/vector/size

[commit-bot: I haz the power, 2016-11-11 12:12:36]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-17 22:30:25]

Description was changed from

==========
[ignition] Fix more -Wsign-compare warnings.

BUG=v8:5614
==========

to

==========
[ignition] Fix more -Wsign-compare warnings.

BUG=v8:5614

Committed: https://crrev.com/2bd1ee96a555ae28d1efeb018d725fb577ae35d8
Cr-Commit-Position: refs/heads/master@{#40918}
==========

[commit-bot: I haz the power, 2016-11-17 22:30:26]

Patchset 2 (id:??) landed as
https://crrev.com/2bd1ee96a555ae28d1efeb018d725fb577ae35d8
Cr-Commit-Position: refs/heads/master@{#40918}