Encapsulating Type information in the CompareICStub

Encapsulating Type information in the CompareICStub

Encapsulate type information in a convenient wrapper instead of storing it in a naked bitfield. This especially facilitates transitioning to a new state and converting from/to the extraICState representation. Additionally cleaning up ToBooleanICStub::Types for consistency.

BUG=
R=svenpanne@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=14704

[Sven Panne, 2013-05-13 14:24:50]

First round of comments...

https://codereview.chromium.org/14862009/diff/1/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/14862009/diff/1/src/code-stubs.cc#newcode459
src/code-stubs.cc:459: void CompareNilICStub::Types::Print(StringStream* stream)
const {
This doesn't produce very readable output when Types contains more than a single
element.

https://codereview.chromium.org/14862009/diff/1/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/14862009/diff/1/src/code-stubs.h#newcode1045
src/code-stubs.h:1045: class Types {
Instead of inventing tons of ad-hoc helper functions, publicly derive from
EnumSet<Type,byte> and add just Print(StringStream*), perhaps FullCompare().
Nothing else is really needed.

https://codereview.chromium.org/14862009/diff/1/src/code-stubs.h#newcode1135
src/code-stubs.h:1135: virtual Code::ExtraICState GetExtraICState() {
Use the BitField template instead of doing the bit fiddling by hand.

https://codereview.chromium.org/14862009/diff/1/src/code-stubs.h#newcode1146
src/code-stubs.h:1146: 
Do we really need all those tiny helper functions below? One reason could be
avoiding "train wrecks" like foo()->bar()->baz(), but I am not sure if this is
the case here.

https://codereview.chromium.org/14862009/diff/1/src/code-stubs.h#newcode1152
src/code-stubs.h:1152: const Types GetTypes() const { return types_; }
Why 2 consts?

https://codereview.chromium.org/14862009/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/14862009/diff/1/src/hydrogen.cc#newcode10687
src/hydrogen.cc:10687: if (nil == kNullValue) {
Using a ternary ?: is clearer here.

[oliv, 2013-05-13 18:03:58]

https://codereview.chromium.org/14862009/diff/1/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/14862009/diff/1/src/code-stubs.cc#newcode459
src/code-stubs.cc:459: void CompareNilICStub::Types::Print(StringStream* stream)
const {
On 2013/05/13 14:24:50, Sven Panne wrote:
> This doesn't produce very readable output when Types contains more than a
single
> element.

ok, done

https://codereview.chromium.org/14862009/diff/1/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/14862009/diff/1/src/code-stubs.h#newcode1045
src/code-stubs.h:1045: class Types {
On 2013/05/13 14:24:50, Sven Panne wrote:
> Instead of inventing tons of ad-hoc helper functions, publicly derive from
> EnumSet<Type,byte> and add just Print(StringStream*), perhaps FullCompare().
> Nothing else is really needed.

Good point -> Done

https://codereview.chromium.org/14862009/diff/1/src/code-stubs.h#newcode1135
src/code-stubs.h:1135: virtual Code::ExtraICState GetExtraICState() {
On 2013/05/13 14:24:50, Sven Panne wrote:
> Use the BitField template instead of doing the bit fiddling by hand.

They are BitFields, except for the types, since those are just the lsb, so no
need to encode anything there.
Or maybe i don't fully get the bitField templates?

https://codereview.chromium.org/14862009/diff/1/src/code-stubs.h#newcode1146
src/code-stubs.h:1146: 
On 2013/05/13 14:24:50, Sven Panne wrote:
> Do we really need all those tiny helper functions below? One reason could be
> avoiding "train wrecks" like foo()->bar()->baz(), but I am not sure if this is
> the case here.

i mostly introduced them to make the callees code more readable, but i agree,
having two different accessors is bad

https://codereview.chromium.org/14862009/diff/1/src/code-stubs.h#newcode1152
src/code-stubs.h:1152: const Types GetTypes() const { return types_; }
On 2013/05/13 14:24:50, Sven Panne wrote:
> Why 2 consts?

1. the returned types shall be const
2. the method does not change the stub itself -> const

https://codereview.chromium.org/14862009/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/14862009/diff/1/src/hydrogen.cc#newcode10687
src/hydrogen.cc:10687: if (nil == kNullValue) {
On 2013/05/13 14:24:50, Sven Panne wrote:
> Using a ternary ?: is clearer here.

ok

[Sven Panne, 2013-05-14 09:18:42]

Adding Danno because of my question about CompareNilICStub::Record.

https://codereview.chromium.org/14862009/diff/8001/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/14862009/diff/8001/src/code-stubs.cc#newcode413
src/code-stubs.cc:413: if (nil_value_ == kNullValue) {
Use a ternary. More importantly: Why do we *set* types_ here and *Add* below? It
is unclear to me why we have to handle this differently. If it is correct,
please add a comment. (Yes, it has benn there  before in a different form, but
nevertheless...)

https://codereview.chromium.org/14862009/diff/8001/src/code-stubs.cc#newcode443
src/code-stubs.cc:443: if (nil_value_ == kNullValue) {
Use a ternary ?: here...

https://codereview.chromium.org/14862009/diff/8001/src/code-stubs.cc#newcode448
src/code-stubs.cc:448: if (equality_kind_ == kStrictEquality) {
... and here, and merge the stream->Add() calls. More concise and easier to
read.

https://codereview.chromium.org/14862009/diff/8001/src/code-stubs.cc#newcode462
src/code-stubs.cc:462: stream->Remove(1);
Although it looks like a cunning idea at first, removing things from a stream is
a bad idea conceptually. I want to refactor things to clearly distinguish
between the conversion of an object to a sequence of characters and the
character sink itself. This kind of hack would be counter-productive. Just keep
track of the fact if you need to emit a separator by hand. C++ sucks:
http://hackage.haskell.org/packages/archive/base/latest/doc/html/Data-List.ht...
:-/

"What happens in a stream, stays in a stream!". ;-)

https://codereview.chromium.org/14862009/diff/8001/src/code-stubs.cc#newcode582
src/code-stubs.cc:582: stream->Remove(1);
Same comment as in the other Print() function.

https://codereview.chromium.org/14862009/diff/8001/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/14862009/diff/8001/src/code-stubs.h#newcode1063
src/code-stubs.h:1063: static const Types FullCompare() {
No need for "const" here, we have an implicit copy, anyway.

https://codereview.chromium.org/14862009/diff/8001/src/code-stubs.h#newcode1071
src/code-stubs.h:1071: bool IsFullCompare() const {
No need for this function, we have FullCompare() and operator==().

https://codereview.chromium.org/14862009/diff/8001/src/code-stubs.h#newcode1075
src/code-stubs.h:1075: void SetTo(Type type);
We don't need this, pretending that Types is immutable and just assigning is
clearer. If you like, you could add a constructor/factory method for a singleton
to polish the call sites a bit, but that's up to you.

https://codereview.chromium.org/14862009/diff/8001/src/code-stubs.h#newcode1076
src/code-stubs.h:1076: byte ToByte() const { return ToIntegral(); }
Remove this, just use ToIntegral().

https://codereview.chromium.org/14862009/diff/8001/src/code-stubs.h#newcode1147
src/code-stubs.h:1147: const Types GetTypes() const { return types_; }
The first 'const' doesn't really make sense: It doesn't force the callee to
assign the result to e.g. a 'const Types foo'. Furthermore, we implicitly copy
types_, anyway.

https://codereview.chromium.org/14862009/diff/8001/src/code-stubs.h#newcode1844
src/code-stubs.h:1844: byte ToByte() const { return ToIntegral(); }
Just using ToIntegral() is easier here, too. (I know this has been there before
your change... :-)

[oliv, 2013-05-14 13:28:11]

https://codereview.chromium.org/14862009/diff/8001/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/14862009/diff/8001/src/code-stubs.cc#newcode462
src/code-stubs.cc:462: stream->Remove(1);
On 2013/05/14 09:18:42, Sven Panne wrote:
> Although it looks like a cunning idea at first, removing things from a stream
is
> a bad idea conceptually. I want to refactor things to clearly distinguish
> between the conversion of an object to a sequence of characters and the
> character sink itself. This kind of hack would be counter-productive. Just
keep
> track of the fact if you need to emit a separator by hand. 

ok, ic
> C++ sucks:
>
http://hackage.haskell.org/packages/archive/base/latest/doc/html/Data-List.ht...
> :-/

i know:
http://ruby-doc.org/core-2.0/Array.html#method-i-join
;)

https://codereview.chromium.org/14862009/diff/8001/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/14862009/diff/8001/src/code-stubs.h#newcode1844
src/code-stubs.h:1844: byte ToByte() const { return ToIntegral(); }
On 2013/05/14 09:18:42, Sven Panne wrote:
> Just using ToIntegral() is easier here, too. (I know this has been there
before
> your change... :-)

This is still called from platform dependant code, which i anyway want to
completely remove in a later cl. so i'm not gonna remove that in this cl

[Sven Panne, 2013-05-15 09:13:33]

lgtm

[oliv, 2013-05-16 10:59:31]

Committed patchset #3 manually as r14704 (presubmit successful).