Nitpicking: please include bug number into issue description either as BUG=907
or BUG=http://code.google.com/p/v8/issues/detail?id=907 This helps to track correspondence between issues and revisions. -- Vyacheslav Egorov On Tue, Oct 26, 2010 at 11:14 AM, <[email protected]> wrote: > > http://codereview.chromium.org/4004006/diff/1/2 > File src/arm/codegen-arm.cc (right): > > http://codereview.chromium.org/4004006/diff/1/2#newcode48 > src/arm/codegen-arm.cc:48: #include "codegen.h" > We usually alphabetize includes. Sometimes it hides implicit > dependencies, sometimes it reveals them, but at least its systematic. > > We also have that xxx-inl.h always includes xxx.h, so including > codegen-inl.h is enough to get codegen.h and there's no need for this > new include. > > http://codereview.chromium.org/4004006/diff/1/3 > File src/arm/full-codegen-arm.cc (right): > > http://codereview.chromium.org/4004006/diff/1/3#newcode356 > src/arm/full-codegen-arm.cc:356: } > Our usual style calls for two blank lines between most top-level > definitions. > > http://codereview.chromium.org/4004006/diff/1/7 > File src/codegen.h (right): > > http://codereview.chromium.org/4004006/diff/1/7#newcode90 > src/codegen.h:90: void CalculateEmitStore(ObjectLiteral *); > We typically avoid these free functions. In this case, it doesn't have > anything specific to do with this code generator, and properly belongs > as a member function on ObjectLiteral. > > http://codereview.chromium.org/4004006/diff/1/8 > File src/full-codegen.cc (right): > > http://codereview.chromium.org/4004006/diff/1/8#newcode41 > src/full-codegen.cc:41: > This, on the other hand, is too much vertical whitespace! > > http://codereview.chromium.org/4004006/show > > -- > v8-dev mailing list > [email protected] > http://groups.google.com/group/v8-dev > -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
