Fix compile warning [-Wtype-limits]

Fix compile warning [-Wtype-limits]

This fixes warning on android_arm build.
Previously the compiler complained about a check that can never be true.
See second check below(index is size_t type, FIRST_SPACE=0):
if (index > i::LAST_SPACE || index < i::FIRST_SPACE)
And make the code easy to understand.

BUG=

Committed: https://crrev.com/e23d8bc7e790519254a4ce2865a2a9461d02ecce
Cr-Commit-Position: refs/heads/master@{#29042}

[sejunho, 2015-06-04 06:26:13]

sejunho@gmail.com changed reviewers:
+ vogelheim@chromium.org

[sejunho, 2015-06-04 06:26:13]

PTAL

[vogelheim, 2015-06-05 08:30:43]

https://codereview.chromium.org/1155043005/diff/1/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/1155043005/diff/1/src/api.cc#newcode7214
src/api.cc:7214: if (index > i::LAST_SPACE || (i::FIRST_SPACE > 0 && index <
i::FIRST_SPACE))
The way I read globals.h, i::FIRST_SPACE would usually be 0, meaning this
effectively eliminates the 2nd condition.
Also, I'm not really sure why this would be the right place to make an assertion
about the i::FIRST_SPACE constant in the first place.

Would you care to explain a bit more what you're trying to do? I'm afraid I
don't quite get it.

[sejunho, 2015-06-05 09:05:38]

On 2015/06/05 08:30:43, vogelheim wrote:
> https://codereview.chromium.org/1155043005/diff/1/src/api.cc
> File src/api.cc (right):
> 
> https://codereview.chromium.org/1155043005/diff/1/src/api.cc#newcode7214
> src/api.cc:7214: if (index > i::LAST_SPACE || (i::FIRST_SPACE > 0 && index <
> i::FIRST_SPACE))
> The way I read globals.h, i::FIRST_SPACE would usually be 0, meaning this
> effectively eliminates the 2nd condition.

In the android_arm build(ndk-r10c, r10d), the compiler output the warning
message like this:
warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
   if (index > i::LAST_SPACE || index < i::FIRST_SPACE)
                                           ^

> Also, I'm not really sure why this would be the right place to make an
assertion
> about the i::FIRST_SPACE constant in the first place.
> 
> Would you care to explain a bit more what you're trying to do? I'm afraid I
> don't quite get it.

I think it is just simple solution for the warning. Maybe the meaningless
comparison consider that constant about the i::FIRST_SPACE would be changed.
I want just fix the warning without breaking original intent.

[vogelheim, 2015-06-08 16:42:38]

On 2015/06/05 09:05:38, sejunho wrote:
> On 2015/06/05 08:30:43, vogelheim wrote:
> > https://codereview.chromium.org/1155043005/diff/1/src/api.cc
> > File src/api.cc (right):
> > 
> > https://codereview.chromium.org/1155043005/diff/1/src/api.cc#newcode7214
> > src/api.cc:7214: if (index > i::LAST_SPACE || (i::FIRST_SPACE > 0 && index <
> > i::FIRST_SPACE))
> > The way I read globals.h, i::FIRST_SPACE would usually be 0, meaning this
> > effectively eliminates the 2nd condition.
> 
> In the android_arm build(ndk-r10c, r10d), the compiler output the warning
> message like this:
> warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
>    if (index > i::LAST_SPACE || index < i::FIRST_SPACE)
>                                            ^
> 
> > Also, I'm not really sure why this would be the right place to make an
> assertion
> > about the i::FIRST_SPACE constant in the first place.
> > 
> > Would you care to explain a bit more what you're trying to do? I'm afraid I
> > don't quite get it.
> 
> I think it is just simple solution for the warning. Maybe the meaningless
> comparison consider that constant about the i::FIRST_SPACE would be changed.
> I want just fix the warning without breaking original intent.

Thanks for the explanation. I get it now.

It's not super readable, though: Previously the compiler complained about a
check that can never be true, and now we make it never-be-true in a somewhat
more obtuse fashion.

I asked around, and one proposal would be to add a compile time assert about
FIRST_SPACE, and then drop the second clause entirely. Like so:

COMPILE_ASSERT(i::FIRST_SPACE == 0, first_space_assumed_to_be_zero);
if (index > i::LAST_SPACE) ...

Would that work?


Reference for COMPILE_ASSERT:
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/base/macros...



Also, sorry for being a bit pedantic. :-|

[sejunho, 2015-06-09 01:12:44]

On 2015/06/08 16:42:38, vogelheim wrote:
> On 2015/06/05 09:05:38, sejunho wrote:
> > On 2015/06/05 08:30:43, vogelheim wrote:
> > > https://codereview.chromium.org/1155043005/diff/1/src/api.cc
> > > File src/api.cc (right):
> > > 
> > > https://codereview.chromium.org/1155043005/diff/1/src/api.cc#newcode7214
> > > src/api.cc:7214: if (index > i::LAST_SPACE || (i::FIRST_SPACE > 0 && index
<
> > > i::FIRST_SPACE))
> > > The way I read globals.h, i::FIRST_SPACE would usually be 0, meaning this
> > > effectively eliminates the 2nd condition.
> > 
> > In the android_arm build(ndk-r10c, r10d), the compiler output the warning
> > message like this:
> > warning: comparison of unsigned expression < 0 is always false
[-Wtype-limits]
> >    if (index > i::LAST_SPACE || index < i::FIRST_SPACE)
> >                                            ^
> > 
> > > Also, I'm not really sure why this would be the right place to make an
> > assertion
> > > about the i::FIRST_SPACE constant in the first place.
> > > 
> > > Would you care to explain a bit more what you're trying to do? I'm afraid
I
> > > don't quite get it.
> > 
> > I think it is just simple solution for the warning. Maybe the meaningless
> > comparison consider that constant about the i::FIRST_SPACE would be changed.
> > I want just fix the warning without breaking original intent.
> 
> Thanks for the explanation. I get it now.
> 
> It's not super readable, though: Previously the compiler complained about a
> check that can never be true, and now we make it never-be-true in a somewhat
> more obtuse fashion.
> 
> I asked around, and one proposal would be to add a compile time assert about
> FIRST_SPACE, and then drop the second clause entirely. Like so:
> 
> COMPILE_ASSERT(i::FIRST_SPACE == 0, first_space_assumed_to_be_zero);
> if (index > i::LAST_SPACE) ...
> 
> Would that work?
> 
> 
> Reference for COMPILE_ASSERT:
>
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/base/macros...
> 
> 
> 
> Also, sorry for being a bit pedantic. :-|


Thank you very much for review(and thank you for read inconvenience sentences
:-)).
Using compile assert work well. I uploaded the patch.

[sejunho, 2015-06-09 01:15:25]

On 2015/06/09 01:12:44, sejunho wrote:
> On 2015/06/08 16:42:38, vogelheim wrote:
> > On 2015/06/05 09:05:38, sejunho wrote:
> > > On 2015/06/05 08:30:43, vogelheim wrote:
> > > > https://codereview.chromium.org/1155043005/diff/1/src/api.cc
> > > > File src/api.cc (right):
> > > > 
> > > > https://codereview.chromium.org/1155043005/diff/1/src/api.cc#newcode7214
> > > > src/api.cc:7214: if (index > i::LAST_SPACE || (i::FIRST_SPACE > 0 &&
index
> <
> > > > i::FIRST_SPACE))
> > > > The way I read globals.h, i::FIRST_SPACE would usually be 0, meaning
this
> > > > effectively eliminates the 2nd condition.
> > > 
> > > In the android_arm build(ndk-r10c, r10d), the compiler output the warning
> > > message like this:
> > > warning: comparison of unsigned expression < 0 is always false
> [-Wtype-limits]
> > >    if (index > i::LAST_SPACE || index < i::FIRST_SPACE)
> > >                                            ^
> > > 
> > > > Also, I'm not really sure why this would be the right place to make an
> > > assertion
> > > > about the i::FIRST_SPACE constant in the first place.
> > > > 
> > > > Would you care to explain a bit more what you're trying to do? I'm
afraid
> I
> > > > don't quite get it.
> > > 
> > > I think it is just simple solution for the warning. Maybe the meaningless
> > > comparison consider that constant about the i::FIRST_SPACE would be
changed.
> > > I want just fix the warning without breaking original intent.
> > 
> > Thanks for the explanation. I get it now.
> > 
> > It's not super readable, though: Previously the compiler complained about a
> > check that can never be true, and now we make it never-be-true in a somewhat
> > more obtuse fashion.
> > 
> > I asked around, and one proposal would be to add a compile time assert about
> > FIRST_SPACE, and then drop the second clause entirely. Like so:
> > 
> > COMPILE_ASSERT(i::FIRST_SPACE == 0, first_space_assumed_to_be_zero);
> > if (index > i::LAST_SPACE) ...
> > 
> > Would that work?
> > 
> > 
> > Reference for COMPILE_ASSERT:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/base/macros...
> > 
> > 
> > 
> > Also, sorry for being a bit pedantic. :-|
> 
> 
> Thank you very much for review(and thank you for read inconvenience sentences
> :-)).
> Using compile assert work well. I uploaded the patch.

Also, i attached the compile result when the expression checking is failed.

==>
CXX(target)
/home/junho0924.seo/devel/v8_latest/v8/out/android_arm.release/obj.target/v8_base/src/api.o
CXX(host)
/home/junho0924.seo/devel/v8_latest/v8/out/android_arm.release/obj.host/v8_base/src/api.o
In file included from .././src/globals.h:15:0,
                 from .././src/allocation.h:8,
                 from .././src/v8.h:32,
                 from .././src/api.h:8,
                 from ../src/api.cc:5:
../src/api.cc: In member function 'bool
v8::Isolate::GetHeapSpaceStatistics(v8::HeapSpaceStatistics*, size_t)':
.././src/base/macros.h:125:35: error: static assertion failed:
first_space_assumed_to_be_zero
 #define COMPILE_ASSERT(expr, msg) static_assert(expr, #msg)
                                   ^
../src/api.cc:7257:3: note: in expansion of macro 'COMPILE_ASSERT'
   COMPILE_ASSERT(i::FIRST_SPACE == 1, first_space_assumed_to_be_zero);
   ^
At global scope:
cc1plus: warning: unrecognized command line option "-Wno-format-pedantic"
[enabled by default]
make[2]: ***
[/home/junho0924.seo/devel/v8_latest/v8/out/android_arm.release/obj.target/v8_base/src/api.o]
Error 1
make[2]: *** Waiting for unfinished jobs....
../src/api.cc:7257:3: error: static_assert failed
"first_space_assumed_to_be_zero"
  COMPILE_ASSERT(i::FIRST_SPACE == 1, first_space_assumed_to_be_zero);
  ^              ~~~~~~~~~~~~~~~~~~~
.././src/base/macros.h:125:35: note: expanded from macro 'COMPILE_ASSERT'
#define COMPILE_ASSERT(expr, msg) static_assert(expr, #msg)
                                  ^
1 error generated.
make[2]: ***
[/home/junho0924.seo/devel/v8_latest/v8/out/android_arm.release/obj.host/v8_base/src/api.o]
Error 1

[Sven Panne, 2015-06-09 06:44:46]

svenpanne@chromium.org changed reviewers:
+ svenpanne@chromium.org

[Sven Panne, 2015-06-09 06:44:46]

https://codereview.chromium.org/1155043005/diff/20001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/1155043005/diff/20001/src/api.cc#newcode7256
src/api.cc:7256: COMPILE_ASSERT(i::FIRST_SPACE == 0,
first_space_assumed_to_be_zero);
Sorry for chiming in so late, but IMHO this is still far too complicated, just
express what you really want to say explicitly:

bool IsValidAllocationSpace(AllocationSpace space) {
  switch (space) {
    case NEW_SPACE:
    case OLD_SPACE:
    case CODE_SPACE:
    case MAP_SPACE:
    case LO_SPACE:
      return true;
    default:
      return false;
  }
}

(Name/namespace/etc. is open for bikeshedding :-) This is crystal clear,
compiles to 2 instructions in a value context and even 1 instruction in a test
context.

More generally: Looking at the usages, AllocationSpace should really be a class,
not an enum...

[sejunho, 2015-06-09 08:22:27]

https://codereview.chromium.org/1155043005/diff/20001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/1155043005/diff/20001/src/api.cc#newcode7256
src/api.cc:7256: COMPILE_ASSERT(i::FIRST_SPACE == 0,
first_space_assumed_to_be_zero);
On 2015/06/09 06:44:46, Sven Panne wrote:
> Sorry for chiming in so late, but IMHO this is still far too complicated, just
> express what you really want to say explicitly:
> 
> bool IsValidAllocationSpace(AllocationSpace space) {
>   switch (space) {
>     case NEW_SPACE:
>     case OLD_SPACE:
>     case CODE_SPACE:
>     case MAP_SPACE:
>     case LO_SPACE:
>       return true;
>     default:
>       return false;
>   }
> }
> 
> (Name/namespace/etc. is open for bikeshedding :-) This is crystal clear,
> compiles to 2 instructions in a value context and even 1 instruction in a test
> context.
> 
> More generally: Looking at the usages, AllocationSpace should really be a
class,
> not an enum...


Thank you for comment sven.
First, I fixed like this:
if (i::Heap::IsValidAllocationSpace(
       static_cast<i::AllocationSpace>(index)))
    return false;

But, this was very bad because if the index is in
the out of range of AllocationSpace, then process crash.
I think casting size_t to AllocationSpace requires
range checking. 
if (i::FIRST_SPACE <= index && index <= i::LAST_SPACE)

Second, I found heap->GetSpaceName which is using the
integer as argument. I think this is better solution.
What do you think?

if (i::Heap::IsValidAllocationSpace(
       static_cast<int>(index)))

const char* Heap::IsValidAllocationSpace(int idx) {
  switch (idx) {
    case NEW_SPACE:
    case OLD_SPACE:
    case MAP_SPACE:
    case CODE_SPACE:
    case LO_SPACE:
      return true;
    default:
      return false;
  }
}

[sejunho, 2015-06-09 08:26:09]

On 2015/06/09 08:22:27, sejunho wrote:
> https://codereview.chromium.org/1155043005/diff/20001/src/api.cc
> File src/api.cc (right):
> 
> https://codereview.chromium.org/1155043005/diff/20001/src/api.cc#newcode7256
> src/api.cc:7256: COMPILE_ASSERT(i::FIRST_SPACE == 0,
> first_space_assumed_to_be_zero);
> On 2015/06/09 06:44:46, Sven Panne wrote:
> > Sorry for chiming in so late, but IMHO this is still far too complicated,
just
> > express what you really want to say explicitly:
> > 
> > bool IsValidAllocationSpace(AllocationSpace space) {
> >   switch (space) {
> >     case NEW_SPACE:
> >     case OLD_SPACE:
> >     case CODE_SPACE:
> >     case MAP_SPACE:
> >     case LO_SPACE:
> >       return true;
> >     default:
> >       return false;
> >   }
> > }
> > 
> > (Name/namespace/etc. is open for bikeshedding :-) This is crystal clear,
> > compiles to 2 instructions in a value context and even 1 instruction in a
test
> > context.
> > 
> > More generally: Looking at the usages, AllocationSpace should really be a
> class,
> > not an enum...
> 
> 
> Thank you for comment sven.
> First, I fixed like this:
> if (i::Heap::IsValidAllocationSpace(
>        static_cast<i::AllocationSpace>(index)))
>     return false;
> 
> But, this was very bad because if the index is in
> the out of range of AllocationSpace, then process crash.
> I think casting size_t to AllocationSpace requires
> range checking. 
> if (i::FIRST_SPACE <= index && index <= i::LAST_SPACE)
> 
> Second, I found heap->GetSpaceName which is using the
> integer as argument. I think this is better solution.
> What do you think?
> 
> if (i::Heap::IsValidAllocationSpace(
>        static_cast<int>(index)))
> 
> const char* Heap::IsValidAllocationSpace(int idx) {
>   switch (idx) {
>     case NEW_SPACE:
>     case OLD_SPACE:
>     case MAP_SPACE:
>     case CODE_SPACE:
>     case LO_SPACE:
>       return true;
>     default:
>       return false;
>   }
> }

oops, i deleted '!' while editing.
if (!i::Heap::IsValidAllocationSpace(
       static_cast<i::AllocationSpace>(index)))

if (!i::Heap::IsValidAllocationSpace(
       static_cast<int>(index)))

[sejunho, 2015-06-10 03:10:56]

On 2015/06/09 08:26:09, sejunho wrote:
> On 2015/06/09 08:22:27, sejunho wrote:
> > https://codereview.chromium.org/1155043005/diff/20001/src/api.cc
> > File src/api.cc (right):
> > 
> > https://codereview.chromium.org/1155043005/diff/20001/src/api.cc#newcode7256
> > src/api.cc:7256: COMPILE_ASSERT(i::FIRST_SPACE == 0,
> > first_space_assumed_to_be_zero);
> > On 2015/06/09 06:44:46, Sven Panne wrote:
> > > Sorry for chiming in so late, but IMHO this is still far too complicated,
> just
> > > express what you really want to say explicitly:
> > > 
> > > bool IsValidAllocationSpace(AllocationSpace space) {
> > >   switch (space) {
> > >     case NEW_SPACE:
> > >     case OLD_SPACE:
> > >     case CODE_SPACE:
> > >     case MAP_SPACE:
> > >     case LO_SPACE:
> > >       return true;
> > >     default:
> > >       return false;
> > >   }
> > > }
> > > 
> > > (Name/namespace/etc. is open for bikeshedding :-) This is crystal clear,
> > > compiles to 2 instructions in a value context and even 1 instruction in a
> test
> > > context.
> > > 
> > > More generally: Looking at the usages, AllocationSpace should really be a
> > class,
> > > not an enum...
> > 
> > 

I'm sorry for incorrect information. I've mistake when verify
implementation. Please never mind comment #10, #11.

I will upload new patch.

[sejunho, 2015-06-10 03:17:26]

https://codereview.chromium.org/1155043005/diff/20001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/1155043005/diff/20001/src/api.cc#newcode7256
src/api.cc:7256: COMPILE_ASSERT(i::FIRST_SPACE == 0,
first_space_assumed_to_be_zero);
On 2015/06/09 06:44:46, Sven Panne wrote:
> Sorry for chiming in so late, but IMHO this is still far too complicated, just
> express what you really want to say explicitly:
> 
> bool IsValidAllocationSpace(AllocationSpace space) {
>   switch (space) {
>     case NEW_SPACE:
>     case OLD_SPACE:
>     case CODE_SPACE:
>     case MAP_SPACE:
>     case LO_SPACE:
>       return true;
>     default:
>       return false;
>   }
> }
> 
> (Name/namespace/etc. is open for bikeshedding :-) This is crystal clear,
> compiles to 2 instructions in a value context and even 1 instruction in a test
> context.
> 
> More generally: Looking at the usages, AllocationSpace should really be a
class,
> not an enum...

Done.

[sejunho, 2015-06-15 07:22:26]

On 2015/06/10 03:17:26, sejunho wrote:
> https://codereview.chromium.org/1155043005/diff/20001/src/api.cc
> File src/api.cc (right):
> 
> https://codereview.chromium.org/1155043005/diff/20001/src/api.cc#newcode7256
> src/api.cc:7256: COMPILE_ASSERT(i::FIRST_SPACE == 0,
> first_space_assumed_to_be_zero);
> On 2015/06/09 06:44:46, Sven Panne wrote:
> > Sorry for chiming in so late, but IMHO this is still far too complicated,
just
> > express what you really want to say explicitly:
> > 
> > bool IsValidAllocationSpace(AllocationSpace space) {
> >   switch (space) {
> >     case NEW_SPACE:
> >     case OLD_SPACE:
> >     case CODE_SPACE:
> >     case MAP_SPACE:
> >     case LO_SPACE:
> >       return true;
> >     default:
> >       return false;
> >   }
> > }
> > 
> > (Name/namespace/etc. is open for bikeshedding :-) This is crystal clear,
> > compiles to 2 instructions in a value context and even 1 instruction in a
test
> > context.
> > 
> > More generally: Looking at the usages, AllocationSpace should really be a
> class,
> > not an enum...
> 
> Done.

Ptal

[vogelheim, 2015-06-15 16:10:06]

vogelheim@chromium.org changed reviewers:
+ hpayer@chromium.org

[vogelheim, 2015-06-15 16:10:07]

Looks good.

Since this now affects src/heap/*, putting heap owner hpayer@ as reviewer. I'll
be happy with whatever he recommends.

[Hannes Payer (out of office), 2015-06-16 07:24:36]

LGTM, please add a more descriptive description about what you fixed.

[sejunho, 2015-06-16 07:33:45]

Patchset #4 (id:60001) has been deleted

[sejunho, 2015-06-16 07:47:28]

On 2015/06/16 07:24:36, Hannes Payer wrote:
> LGTM, please add a more descriptive description about what you fixed.

done

[sejunho, 2015-06-16 07:49:15]

The CQ bit was checked by sejunho@gmail.com

[commit-bot: I haz the power, 2015-06-16 07:49:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1155043005/40001

[commit-bot: I haz the power, 2015-06-16 08:20:17]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-06-16 08:20:31]

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