Clean up manual bit field usage in PreParserExpression

Clean up manual bit field usage in PreParserExpression

Instead of using an integer value and manual bit-fiddling, use C++'s
support for specifying bit sizes for integral types. This way the bits
used to describe a PreParserExpression are handled by the compiler.

BUG=
LOG=

[aperez, 2014-08-02 09:44:21]

This is one possible approach to avoid the manual, error-prone bit fiddling, as
discussed here:

  https://codereview.chromium.org/410873004/#msg5

[Sven Panne, 2014-08-04 06:55:57]

NOT LGTM.

https://codereview.chromium.org/422363002/diff/20001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/422363002/diff/20001/src/preparser.h#newcode783
src/preparser.h:783: Type type_ : 2;
This actually blows up memory usage instead of reducing it and has several
portability issues. Please use the BitField class from utils.h, like the rest of
v8.

[aperez, 2014-09-03 15:11:55]

On 2014/08/04 06:55:57, Sven Panne wrote:
> NOT LGTM.
> 
> https://codereview.chromium.org/422363002/diff/20001/src/preparser.h
> File src/preparser.h (right):
> 
>
https://codereview.chromium.org/422363002/diff/20001/src/preparser.h#newcode783
> src/preparser.h:783: Type type_ : 2;
> This actually blows up memory usage instead of reducing it and has several
> portability issues. Please use the BitField class from utils.h, like the rest
of
> v8.

First of all, let me say that I do not have any problem at all
changing this to use “BitField”.

But as far as portability is concerned, bitfield integrals in structs are
defined in the C++ specification as a compiler feature (it does not need
runtime support from the standard library) and all compilers must support
them. Of course compilers are free to reorder the location of the fields
or make decisions about their packing, which of course plays badly with
dumping structs with bitfields to a file or to the network — which is a
no-concern as in V8 we are just using “PreParserExpression” internally. 

Could you please elaborate what would be the portability problem here
that I am missing?

[aperez, 2014-09-05 19:43:53]

On 2014/08/04 06:55:57, Sven Panne wrote:
> NOT LGTM.
> 
> https://codereview.chromium.org/422363002/diff/20001/src/preparser.h
> File src/preparser.h (right):
> 
>
https://codereview.chromium.org/422363002/diff/20001/src/preparser.h#newcode783
> src/preparser.h:783: Type type_ : 2;
> This actually blows up memory usage instead of reducing it and has several
> portability issues. Please use the BitField class from utils.h, like the rest
of
> v8.

PTAL, the latest patch set uses BitField.

[Sven Panne, 2014-09-08 10:35:53]

https://codereview.chromium.org/422363002/diff/40001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/422363002/diff/40001/src/preparser.h#newcode823
src/preparser.h:823: if (IsIdentifier()) {
Use a separate predicate and simplify things:

----------------------------------------
return IsBinaryOperation()
       ? IsValidArrowParamListField::decode(code_)
       : (IsIdentifier() && foo(AsIdentifier()));
----------------------------------------

A better name for foo and its implementation is left as an easy exercise for the
reader... >:-)

[aperez, 2014-09-10 14:24:04]

https://codereview.chromium.org/422363002/diff/40001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/422363002/diff/40001/src/preparser.h#newcode823
src/preparser.h:823: if (IsIdentifier()) {
On 2014/09/08 10:35:52, Sven Panne wrote:
> Use a separate predicate and simplify things:
> 
> ----------------------------------------
> return IsBinaryOperation()
>        ? IsValidArrowParamListField::decode(code_)
>        : (IsIdentifier() && foo(AsIdentifier()));
> ----------------------------------------
> 
> A better name for foo and its implementation is left as an easy exercise for
the
> reader... >:-)

Acknowledged.

[aperez, 2014-09-10 15:57:18]

On 2014/09/08 10:35:53, Sven Panne wrote:
> https://codereview.chromium.org/422363002/diff/40001/src/preparser.h
> File src/preparser.h (right):
> 
>
https://codereview.chromium.org/422363002/diff/40001/src/preparser.h#newcode823
> src/preparser.h:823: if (IsIdentifier()) {
> Use a separate predicate and simplify things:
> 
> ----------------------------------------
> return IsBinaryOperation()
>        ? IsValidArrowParamListField::decode(code_)
>        : (IsIdentifier() && foo(AsIdentifier()));
> ----------------------------------------
> 
> A better name for foo and its implementation is left as an easy exercise for
the
> reader... >:-)

Fixed, PTAL :)

[Sven Panne, 2014-10-06 12:22:09]

lgtm

[aperez, 2014-10-06 14:58:56]

On 2014/10/06 12:22:09, Sven Panne wrote:
> lgtm

Can someone land this for me? I do not have committer rights yet, and I supposse
the CQ is still not working (or is it?).

Thanks in advance!

[wingo, 2014-10-06 15:11:44]

On 2014/10/06 14:58:56, aperez wrote:
> On 2014/10/06 12:22:09, Sven Panne wrote:
> > lgtm
> 
> Can someone land this for me? I do not have committer rights yet, and I
supposse
> the CQ is still not working (or is it?).
> 
> Thanks in advance!

I'll take it.

[wingo (chromium), 2014-10-07 09:23:17]

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

[wingo (chromium), 2014-10-07 09:23:17]

Committed patchset #5 manually as 24429 (presubmit successful).

[marja, 2015-03-10 08:45:41]

Post-commit comments...

https://codereview.chromium.org/422363002/diff/80001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/422363002/diff/80001/src/preparser.h#newcode826
src/preparser.h:826: enum Type {
Type and ExpressionType both??? What's up with that?