LGTM. My only real concern is the timing of the commit; I'd prefer not to get this pushed to trunk as part of the 1.0.4 push that I hope we can land real soon (blocked on issue 265).
http://codereview.chromium.org/40324/diff/1/4 File src/api.cc (right): http://codereview.chromium.org/40324/diff/1/4#newcode1950 Line 1950: return v8::Local<v8::Value>(); Couldn't this just be Local<Value>()? There are a lot of v8:: prefix that aren't strictly needed in other places too. http://codereview.chromium.org/40324/diff/1/9 File src/heap.h (right): http://codereview.chromium.org/40324/diff/1/9#newcode198 Line 198: V(hidden_symbol, "...hidden...") \ I think this way of hiding the hidden properties has to be fixed before we can land this change on trunk. I have no worries landing it on bleeding_edge though, but I would prefer it if that could be postponed until we have pushed 1.0.4 (which includes a bunch of compiler changes). http://codereview.chromium.org/40324/diff/1/6 File src/objects.cc (right): http://codereview.chromium.org/40324/diff/1/6#newcode5070 Line 5070: if (!this->HasProperty(key)) { You end up doing two lookups for the hidden properties here. One for the HasProperty and one for the GetProperty. Maybe this isn't a big deal because you'll have to rewrite this anyway when you change the way you hide the properties, but you may want to use a LocalLookup instead. http://codereview.chromium.org/40324/diff/1/6#newcode5073 Line 5073: if (create_if_needed) { I might be inclined to change this to an early bailout check instead: if (!create_if_needed) return Heap::undefined_value() http://codereview.chromium.org/40324 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
