Clean up implementation reading literal constants.

Clean up implementation reading literal constants.

Moves APInt and corresponding convert to float method into the pnacl
translator. Renames APInt to BitcodeInt to be more clear that we aren't referring to LLVM's APInt. Rather, it is the representation in PNaCl bitcode.

BUG=None
R=stichnot@chromium.org

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

[Karl, 2015-11-30 21:00:43]

Description was changed from

==========
Clean up implementation reading floating literals.

Moves APInt and corresponding convert to float method into the pnacl
translator. Renames APInt to SzFloatBits to be more clear on what the
class is used for.

BUG=None
==========

to

==========
Clean up implementation reading floating literals.

Moves APInt and corresponding convert to float method into the pnacl
translator. Renames APInt to SzFloatBits to be more clear on what the
class is used for.

BUG=None
==========

[Karl, 2015-11-30 21:00:43]

kschimpf@google.com changed reviewers:
+ reed.kotler@imgtec.com, stichnot@chromium.org

[Karl, 2015-11-30 21:02:10]

Description was changed from

==========
Clean up implementation reading floating literals.

Moves APInt and corresponding convert to float method into the pnacl
translator. Renames APInt to SzFloatBits to be more clear on what the
class is used for.

BUG=None
==========

to

==========
Clean up implementation reading literal constants.

Moves APInt and corresponding convert to float method into the pnacl
translator. Renames APInt to SzInt to be more clear on what the
class is used for.

BUG=None
==========

[Karl, 2015-11-30 21:02:36]

Fixed description to match patch set 2.

[rkotlerimgtec, 2015-11-30 21:09:54]

On 2015/11/30 21:02:36, Karl wrote:
> Fixed description to match patch set 2.

I would call it VSzInt for variable size integer.

[Jim Stichnoth, 2015-11-30 21:58:11]

otherwise lgtm

https://codereview.chromium.org/1484003003/diff/20001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/1484003003/diff/20001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:169: class SzInt {
Can this go into an anonymous namespace?

And to add some bikeshedding, I would name it something like SzAPInt, to remind
the user of its LLVM origins (presuming the user won't bother to look at the doc
string above).

[rkotlerimgtec, 2015-11-30 22:10:39]

On 2015/11/30 21:58:11, stichnot wrote:
> otherwise lgtm
> 
> https://codereview.chromium.org/1484003003/diff/20001/src/PNaClTranslator.cpp
> File src/PNaClTranslator.cpp (right):
> 
>
https://codereview.chromium.org/1484003003/diff/20001/src/PNaClTranslator.cpp...
> src/PNaClTranslator.cpp:169: class SzInt {
> Can this go into an anonymous namespace?
> 
> And to add some bikeshedding, I would name it something like SzAPInt, to
remind
> the user of its LLVM origins (presuming the user won't bother to look at the
doc
> string above).

I am doing a lot to clean up the doxygen now. I have a big patch coming later
today which fixes a lot of errors in the Doxygen formatting that will make it
easier to preserve this kind of information. I will have more such patches as I
have time that I will follow up with.

I don't think we need to preserve
history like AP in the Subzero namespace to remind people of some historic
connection to LLVM. It's very confusing because AP = Arbitrary Precision and
there is no arbitrary precision here.

The Doxygen should be what they read to learn this kind of thing.

[Karl, 2015-11-30 22:12:23]

Description was changed from

==========
Clean up implementation reading literal constants.

Moves APInt and corresponding convert to float method into the pnacl
translator. Renames APInt to SzInt to be more clear on what the
class is used for.

BUG=None
==========

to

==========
Clean up implementation reading literal constants.

Moves APInt and corresponding convert to float method into the pnacl
translator. Renames APInt to SzAPInt to be more clear that we aren't referring
to LLVM's APInt.

BUG=None
==========

[rkotlerimgtec, 2015-11-30 22:12:47]

rkotlerimgtec@gmail.com changed reviewers:
+ rkotlerimgtec@gmail.com

[rkotlerimgtec, 2015-11-30 22:12:48]

https://codereview.chromium.org/1484003003/diff/20001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/1484003003/diff/20001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:190: assert(BitWidth == sizeof(IntType) * CHAR_BIT);
I think this form of using unions to do integer to float conversions
is supposed to be done using reinterpret_cast nowadays. 
http://en.cppreference.com/w/cpp/language/explicit_cast

[Jim Stichnoth, 2015-11-30 23:20:22]

https://codereview.chromium.org/1484003003/diff/20001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/1484003003/diff/20001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:169: class SzInt {
On 2015/11/30 21:58:11, stichnot wrote:
> Can this go into an anonymous namespace?

Sorry, I see now that it's already in a ginormous anonymous namespace.

https://codereview.chromium.org/1484003003/diff/20001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:190: assert(BitWidth == sizeof(IntType) * CHAR_BIT);
On 2015/11/30 22:12:48, rkotlerimgtec wrote:
> I think this form of using unions to do integer to float conversions
> is supposed to be done using reinterpret_cast nowadays. 
> http://en.cppreference.com/w/cpp/language/explicit_cast
> 

This is a good point, unions aren't supposed to be used for type punning.

It does seem like reinterpret_cast should work:

  return reinterpret_cast<FpType&>(Val);

[Karl, 2015-12-01 00:00:41]

Description was changed from

==========
Clean up implementation reading literal constants.

Moves APInt and corresponding convert to float method into the pnacl
translator. Renames APInt to SzAPInt to be more clear that we aren't referring
to LLVM's APInt.

BUG=None
==========

to

==========
Clean up implementation reading literal constants.

Moves APInt and corresponding convert to float method into the pnacl
translator. Renames APInt to BitcodeInt to be more clear that we aren't
referring to LLVM's APInt. Rather, it is the representation in PNaCl bitcode.

BUG=None
==========

[Karl, 2015-12-01 00:00:49]

https://codereview.chromium.org/1484003003/diff/20001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/1484003003/diff/20001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:190: assert(BitWidth == sizeof(IntType) * CHAR_BIT);
On 2015/11/30 23:20:22, stichnot wrote:
> On 2015/11/30 22:12:48, rkotlerimgtec wrote:
> > I think this form of using unions to do integer to float conversions
> > is supposed to be done using reinterpret_cast nowadays. 
> > http://en.cppreference.com/w/cpp/language/explicit_cast
> > 
> 
> This is a good point, unions aren't supposed to be used for type punning.
> 
> It does seem like reinterpret_cast should work:
> 
>   return reinterpret_cast<FpType&>(Val);

Done.

[Karl, 2015-12-01 19:24:57]

Description was changed from

==========
Clean up implementation reading literal constants.

Moves APInt and corresponding convert to float method into the pnacl
translator. Renames APInt to BitcodeInt to be more clear that we aren't
referring to LLVM's APInt. Rather, it is the representation in PNaCl bitcode.

BUG=None
==========

to

==========
Clean up implementation reading literal constants.

Moves APInt and corresponding convert to float method into the pnacl
translator. Renames APInt to BitcodeInt to be more clear that we aren't
referring to LLVM's APInt. Rather, it is the representation in PNaCl bitcode.

BUG=None
R=stichnot@chromium.org

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

[Karl, 2015-12-01 19:24:59]

Committed patchset #3 (id:40001) manually as
a5295b07896b394092bc00307b2256dda3722e2e (presubmit successful).