I should say, if you introduce the flag and change the constant names to obey Google C++ coding style, it LGTM.
On Wed, Mar 3, 2010 at 6:15 AM, Kevin Millikin <[email protected]>wrote: > 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
