I thought we had discussed making this hidden behind a flag for now? Did we decide not to do that?
I would guard your syntax checking with the flag for now (even the extra fields in Expression). So far it's all pretty cheap in time/space, but you could imagine it getting more intensive. It will also help you identify which parts to back out without affecting the current state of affairs, should you ultimately choose to do that (e.g., if it becomes subsumed by the fast code generator). If you introduce the flag, it LGTM. On Tue, Mar 2, 2010 at 3:33 PM, <[email protected]> wrote: > > http://codereview.chromium.org/660372/diff/1004/1005 > > File src/ast.h (right): > > http://codereview.chromium.org/660372/diff/1004/1005#newcode238 > src/ast.h:238: bitfields_ = (bitfields_ & ~SideEffectFreeField::mask()) > | > It's a little weird that there are three different ways of writing this > used in this same snippet (two have different indenation, one has broken > it into two statements). > > http://codereview.chromium.org/660372/diff/1004/1005#newcode268 > src/ast.h:268: static const int SideEffectFreeFieldStart = 0; > These constants don't help anything. They're used in one place in a > context that should be obvious. Write: > > // Bitfields are <type, start, size>. > class SideEffectFreeField: public BitField<bool, 0, 1>; > class ExpressionSizeField: public BitField<int, 1, 5>; > class StackHeightField: public BitField<int, 32 - 6>; > > If you want the constants, please use the coding style > "kSideEffectFreeeFieldStart". > > > http://codereview.chromium.org/660372 > -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
