LGTM with a bunch of nits.
https://chromiumcodereview.appspot.com/11365221/diff/1/src/property.h File src/property.h (right): https://chromiumcodereview.appspot.com/11365221/diff/1/src/property.h#newcode135 src/property.h:135: // Hold a property index value distinguishing if it is a field index or an nit: s/Hold/Holds/ https://chromiumcodereview.appspot.com/11365221/diff/1/src/property.h#newcode146 src/property.h:146: bool IsFieldIndex() { return (index_ & kHeaderIndexBit) == 0; } nit: I think for this and the next three methods, unix_hacker_style names would be appropriate, as they're essentially simple getters. https://chromiumcodereview.appspot.com/11365221/diff/1/src/property.h#newcode158 src/property.h:158: int RawIndex() { return index_; } I don't see this ever getting called. https://chromiumcodereview.appspot.com/11365221/diff/1/src/property.h#newcode167 src/property.h:167: PropertyIndex(int index, bool isHeaderBased) : nit1: ':' should be on the second line (see e.g. the LookupResult constructor right below). nit2: is_header_based. (Usually, we also prefer enums over bools, but since this method is local to the class, using a bool is OK.) nit3: within the "private:" section, please adhere to the ordering statics -> methods -> fields. https://chromiumcodereview.appspot.com/11365221/diff/1/src/property.h#newcode172 src/property.h:172: nit: two lines of whitespace between top-level definitions https://chromiumcodereview.appspot.com/11365221/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
