Good catch, thanks!
http://codereview.chromium.org/7799026/diff/1/src/objects.cc
File src/objects.cc (right):
http://codereview.chromium.org/7799026/diff/1/src/objects.cc#newcode140
src/objects.cc:140: if (IsSmi()) {
On 2011/08/31 10:15:31, Kevin Millikin wrote:
You can use IsNumber() here and remove the clause for IsHeapNumber()
below.
Done.
http://codereview.chromium.org/7799026/diff/1/src/objects.cc#newcode231
src/objects.cc:231: Object** args[] = { receiver.location(),
name.location() };
On 2011/08/31 10:15:31, Kevin Millikin wrote:
We seem to have a mix of this style and
Handle<Object> args[] = { receiver, name };
Handle<Object> result = CallTrap(..., HandleVector(args,
ARRAY_SIZE(args));
I prefer the Handle version, partly because mentioning
Handle<T>::location()
indicates something special and weird going on and so I prefer to keep
it rare.
Also, seeing Handle in a signature makes it obvious (?) that the
function is in
the handle world and should not be called from the raw pointer world.
I agree. I fully handlified the type of CallTrap. But that seems to
require an ugly reinterpret_cast for args. Is there a reason whys
Execution::Call and friends do not properly type the args array with
Handle<>[]? Or even use a HandleVector, like e.g. Throw() etc? Is that
perhaps worth changing?
http://codereview.chromium.org/7799026/diff/1/src/objects.cc#newcode1882
src/objects.cc:1882: Object* value) {
On 2011/08/31 10:15:31, Kevin Millikin wrote:
Indentation is now off.
Done.
http://codereview.chromium.org/7799026/diff/1/src/objects.cc#newcode1910
src/objects.cc:1910: result->HandlerResult(JSProxy::cast(pt));
On 2011/08/31 10:15:31, Kevin Millikin wrote:
I like to write
return result->HandlerResult(JSProxy::cast(pt));
I know. :) Done.
http://codereview.chromium.org/7799026/diff/1/src/objects.cc#newcode2295
src/objects.cc:2295: JSReceiver* receiver_raw, String* name_raw) {
On 2011/08/31 10:15:31, Kevin Millikin wrote:
Also one parameter per line here.
Done.
http://codereview.chromium.org/7799026/diff/1/src/objects.cc#newcode2329
src/objects.cc:2329: const char* name, Object* derived, int argc,
Object*** args) {
On 2011/08/31 10:15:31, Kevin Millikin wrote:
I think there's a GC hazard here. derived is a raw pointer, and
looking up a
symbol in the symbol table can cause allocation and thus GC (it may
have to
allocate the symbol and may also have to grow the symbol table).
Change derived to Handle<Object> in the signature?
Done.
http://codereview.chromium.org/7799026/diff/1/src/objects.cc#newcode2353
src/objects.cc:2353: MaybeObject*
JSObject::SetPropertyForResult(LookupResult* result,
On 2011/08/31 10:15:31, Kevin Millikin wrote:
I think SetPropertyForResult needs a better name but I'm at a loss for
ideas.
Same here. IMHO some bigger clean-up of the whole Get/Set interface is
in order soon anyway, I'd leave it for that.
http://codereview.chromium.org/7799026/diff/1/src/objects.cc#newcode2377
src/objects.cc:2377: JSObject* object = JSObject::cast(this);
On 2011/08/31 10:15:31, Kevin Millikin wrote:
Isn't this already JSObject*?
Right. I had moved the method to JSReceiver, but undid that and
overlooked this.
http://codereview.chromium.org/7799026/diff/1/src/objects.cc#newcode2411
src/objects.cc:2411: Handle<Object> result = proxy->CallTrap(
On 2011/08/31 10:15:31, Kevin Millikin wrote:
There's also a GC hazard here. CallTrap can silently perform GC and
there's no
way for the caller of this function to find that out.
Argh, of course. The good news is that the only caller is
JSReceiver::SetProperty, which is already documented to potentially
cause GC.
The bad news is that a GC might invalidate `result' -- specifically,
result->holder, which is used for a number of methods on LookupResult
below, so it's not enough to handlify the holder separately from
`result'.
I think I can get around that problem here by fully separating the code
paths (in this case, this only required duplicating the !IsFound case).
But in general, is there a way to properly deal with GC vs LookupResult?
http://codereview.chromium.org/7799026/diff/1/src/objects.h
File src/objects.h (right):
http://codereview.chromium.org/7799026/diff/1/src/objects.h#newcode6613
src/objects.h:6613: int argc, Object*** args);
On 2011/08/31 10:15:31, Kevin Millikin wrote:
We consistently use one line per parameter in declarations that don't
fit on a
single line. Even when they're related like here.
Done.
http://codereview.chromium.org/7799026/diff/1/src/runtime.cc
File src/runtime.cc (right):
http://codereview.chromium.org/7799026/diff/1/src/runtime.cc#newcode4496
src/runtime.cc:4496: if (result) return isolate->heap()->true_value();
On 2011/08/31 10:15:31, Kevin Millikin wrote:
You can also write this without the if as:
return isolate->heap()->ToBoolean(result);
Done.
http://codereview.chromium.org/7799026/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
