Thanks a lot, Kasper!
http://codereview.chromium.org/155635/diff/8/1010 File include/v8.h (right): http://codereview.chromium.org/155635/diff/8/1010#newcode1118 Line 1118: void SetPointerInInternalField(int index, void* ptr); On 2009/07/17 08:06:02, Kasper Lund wrote: > ptr -> value? Done. http://codereview.chromium.org/155635/diff/8/1011 File src/api.cc (right): http://codereview.chromium.org/155635/diff/8/1011#newcode2457 Line 2457: if (reinterpret_cast<uintptr_t>(pointer) & 0x1 == 0) { On 2009/07/17 08:06:02, Kasper Lund wrote: > Why not just say: if (pointer->IsSmi())? Done. http://codereview.chromium.org/155635/diff/8/1011#newcode2462 Line 2462: // Read from uninitialzied field On 2009/07/17 08:06:02, Kasper Lund wrote: > There's also a chance of getting a non-proxy object out. You need to deal with > that instead of checking for undefined. I would say if (!pointer->IsProxy()) > return NULL; Good catch. I've added an ASSERT to check that it should be either undefined or proxy---setting handles and reading them as pointers might be not the best use for this API :) http://codereview.chromium.org/155635/diff/8/1011#newcode2484 Line 2484: void v8::Object::SetPointerInInternalField(int index, void* ptr) { On 2009/07/17 08:06:02, Kasper Lund wrote: > Rename ptr to value? Done. http://codereview.chromium.org/155635/diff/8/1011#newcode2486 Line 2486: if (reinterpret_cast<uintptr_t>(ptr) & 0x1 == 0) { On 2009/07/17 08:06:02, Kasper Lund wrote: > How about if (reinterpret_cast<Object*>(ptr)->IsSmi())? Done. I must admit though, that I find explicit alignment checks easier to understand. http://codereview.chromium.org/155635/diff/8/1011#newcode2487 Line 2487: // Aligned pointer, store as is On 2009/07/17 08:06:02, Kasper Lund wrote: > Terminate comment with . Done. http://codereview.chromium.org/155635/diff/8/1011#newcode2490 Line 2490: i::Proxy* proxy = *i::Factory::NewProxy(reinterpret_cast<i::Address>(ptr)); On 2009/07/17 08:14:38, Kasper Lund wrote: > Maybe we should always allocate this in new space? Done. http://codereview.chromium.org/155635/diff/8/1011#newcode2491 Line 2491: ASSERT(reinterpret_cast<uintptr_t>(proxy) & 0x1 == 1); On 2009/07/17 08:06:02, Kasper Lund wrote: > ASSERT(!proxy->IsSmi())? Maybe not really needed though. Done. http://codereview.chromium.org/155635 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
