Subzero: Fix g++ warnings.

Subzero: Fix g++ warnings.

This tries to use the same -W options that the buildbots use, to reduce the amount of warning spam in the logs.

BUG= none
R=jpp@chromium.org

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=b0051dfaca6e957fa62ab61c0ef2a42df7964d37

[Jim Stichnoth, 2016-01-12 17:07:56]

Description was changed from

==========
Subzero: Fix g++ warnings.

BUG= none
==========

to

==========
Subzero: Fix g++ warnings.

This tries to use the same -W options that the buildbots use, to reduce the
amount of warning spam in the logs.

BUG= none
==========

[Jim Stichnoth, 2016-01-12 17:07:56]

stichnot@chromium.org changed reviewers:
+ jpp@chromium.org, kschimpf@google.com, sehr@chromium.org

[Jim Stichnoth, 2016-01-12 17:10:02]

To get a feel for the warnings being fixed, try applying just the
Makefile.standalone change, then:

  make -f Makefile.standalone GPLUSPLUS=1 clean
  make -f Makefile.standalone -j4 -k GPLUSPLUS=1 compile_only

[John, 2016-01-12 18:09:07]

lgtm

https://codereview.chromium.org/1575873006/diff/1/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1575873006/diff/1/src/IceAssemblerARM32.cpp#n...
src/IceAssemblerARM32.cpp:390: llvm_unreachable("(silence g++ warning)");
This is surprising to me. g++ can be so annoying at times.

https://codereview.chromium.org/1575873006/diff/1/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1575873006/diff/1/src/IceInstARM32.cpp#newcod...
src/IceInstARM32.cpp:350: const uint32_t I32 = bit_copy<uint32_t>(F32);
this method (bit_copy) is "poorly" named. I thought there was an std::bit_copy
and, to my surprise, that does not exist.

When I found it, its implementation seems buggy:  D destination;

template <class D, class S> inline D bit_copy(const S &source) {
  // This use of memcpy is safe: source and destination cannot overlap.
  memcpy(&destination, reinterpret_cast<const void *>(&source),
         sizeof(destination));
  return destination;
}

I would think the size parameter should be

std::min(sizeof(destination), sizeof(source));

The method should also belong to the Ice::Utils namespace. Do you mind changing
these things? I know they are unrelated, but I fear this will never be changed
if we don't change it now.

[Jim Stichnoth, 2016-01-13 19:30:33]

https://codereview.chromium.org/1575873006/diff/1/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1575873006/diff/1/src/IceAssemblerARM32.cpp#n...
src/IceAssemblerARM32.cpp:390: llvm_unreachable("(silence g++ warning)");
On 2016/01/12 18:09:07, John wrote:
> This is surprising to me. g++ can be so annoying at times.

Yep!

I thought to add a default case to the first case, but clang++ complains.

Another possibility is to just "return 0;" here (instead or in addition), and
that sorta works in general for functions that return primitive or pointer type:

#define SILENCE_UNREACHABLE_GCC_WARNING 0
...
return SILENCE_UNREACHABLE_GCC_WARNING;

https://codereview.chromium.org/1575873006/diff/1/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1575873006/diff/1/src/IceInstARM32.cpp#newcod...
src/IceInstARM32.cpp:350: const uint32_t I32 = bit_copy<uint32_t>(F32);
On 2016/01/12 18:09:07, John wrote:
> this method (bit_copy) is "poorly" named. I thought there was an std::bit_copy
> and, to my surprise, that does not exist.
> 
> When I found it, its implementation seems buggy:  D destination;
> 
> template <class D, class S> inline D bit_copy(const S &source) {
>   // This use of memcpy is safe: source and destination cannot overlap.
>   memcpy(&destination, reinterpret_cast<const void *>(&source),
>          sizeof(destination));
>   return destination;
> }
> 
> I would think the size parameter should be
> 
> std::min(sizeof(destination), sizeof(source));
> 
> The method should also belong to the Ice::Utils namespace. Do you mind
changing
> these things? I know they are unrelated, but I fear this will never be changed
> if we don't change it now.

Done.

https://codereview.chromium.org/1575873006/diff/20001/src/IceUtils.h
File src/IceUtils.h (right):

https://codereview.chromium.org/1575873006/diff/20001/src/IceUtils.h#newcode38
src/IceUtils.h:38: /// Check whether an N-bit two's-complement representation
can hold value.
Changes here and below are only indentation changes due to class==>namespace.

[Jim Stichnoth, 2016-01-13 19:39:19]

Description was changed from

==========
Subzero: Fix g++ warnings.

This tries to use the same -W options that the buildbots use, to reduce the
amount of warning spam in the logs.

BUG= none
==========

to

==========
Subzero: Fix g++ warnings.

This tries to use the same -W options that the buildbots use, to reduce the
amount of warning spam in the logs.

BUG= none
R=jpp@chromium.org

Committed:
https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a...
==========

[Jim Stichnoth, 2016-01-13 19:39:20]

Committed patchset #2 (id:20001) manually as
b0051dfaca6e957fa62ab61c0ef2a42df7964d37 (presubmit successful).