Handle CALLBACKS correctly in IsProperty functions.

With transitions in AccessorPairs, it is not enough to look at the PropertyType
alone to decide whether we look at a property or not: For objects with
JavaScript accessors, we have to look into the AccessorPair itself and see if
one of its 2 parts is actually a JavaScript accessor. Therefore, a predicate
with a PropertyType argument alone doesn't make sense anymore, we might need the
associated value, too.

Things are complicated by the fact that the holder in a LookupResult can be
NULL, so we must be careful to retrieve its value only when it is really
needed. To achieve the needed call-by-name semantics, a new Entry is introduced,
which is basically a closure over a DescriptorArray and an index into this array
(C++0x to the rescue!). GCC is clever enough to inline this class, so we pay no
runtime penalty for this abstraction.

It's all a bit ugly, but this is caused by the current structure of Descriptor,
DescriptorArray and LookupResult: Things would be much easier if DescriptorArray
were, well, an array of Descriptors, and LookupResult were a 'Maybe Descriptor'
(in Haskell-terms).

Review URL: https://chromiumcodereview.appspot.com/9466047

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@10847 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

diff --git a/src/objects-inl.h b/src/objects-inl.h
index bc6217ba5ebe66ae613719d5c67c06a91952c363..176be9b6ed903e691eac60f0c668867d8579cd57 100644 (file)
--- a/src/objects-inl.h
+++ b/src/objects-inl.h
@@ -1988,7 +1988,8 @@ AccessorDescriptor* DescriptorArray::GetCallbacks(int descriptor_number) {
 
 
 bool DescriptorArray::IsProperty(int descriptor_number) {
-  return IsRealProperty(GetType(descriptor_number));
+  Entry entry(this, descriptor_number);
+  return IsPropertyDescriptor(&entry);
 }
 
 

diff --git a/src/objects.h b/src/objects.h
index be75faba65c0d58183868a185d7c4e6e3ff523da..4e02d6f19f819c884ccb93800122305848e00476 100644 (file)
--- a/src/objects.h
+++ b/src/objects.h
@@ -2589,6 +2589,20 @@ class DescriptorArray: public FixedArray {
   static const int kMaxNumberOfDescriptors = 1024 + 512;
 
  private:
+  // An entry in a DescriptorArray, represented as an (array, index) pair.
+  class Entry {
+   public:
+    inline explicit Entry(DescriptorArray* descs, int index) :
+        descs_(descs), index_(index) { }
+
+    inline PropertyType type() { return descs_->GetType(index_); }
+    inline Object* GetCallbackObject() { return descs_->GetValue(index_); }
+
+   private:
+    DescriptorArray* descs_;
+    int index_;
+  };
+
   // Conversion from descriptor number to array indices.
   static int ToKeyIndex(int descriptor_number) {
     return descriptor_number+kFirstIndex;
@@ -7919,6 +7933,10 @@ class AccessorPair: public Struct {
     }
   }
 
+  bool ContainsAccessor() {
+    return IsJSAccessor(getter()) || IsJSAccessor(setter());
+  }
+
 #ifdef OBJECT_PRINT
   void AccessorPairPrint(FILE* out = stdout);
 #endif
@@ -7931,6 +7949,15 @@ class AccessorPair: public Struct {
   static const int kSize = kSetterOffset + kPointerSize;
 
  private:
+  // Strangely enough, in addition to functions and harmony proxies, the spec
+  // requires us to consider undefined as a kind of accessor, too:
+  //    var obj = {};
+  //    Object.defineProperty(obj, "foo", {get: undefined});
+  //    assertTrue("foo" in obj);
+  bool IsJSAccessor(Object* obj) {
+    return obj->IsSpecFunction() || obj->IsUndefined();
+  }
+
   DISALLOW_IMPLICIT_CONSTRUCTORS(AccessorPair);
 };
 

diff --git a/src/property-details.h b/src/property-details.h
index 81f521a627ac77457675ef9fb1f4dd8e55dfaa37..c79aa969d344d1e795d79d9deb63329561c90fd3 100644 (file)
--- a/src/property-details.h
+++ b/src/property-details.h
@@ -73,26 +73,6 @@ enum PropertyType {
 };
 
 
-inline bool IsRealProperty(PropertyType type) {
-  switch (type) {
-    case NORMAL:
-    case FIELD:
-    case CONSTANT_FUNCTION:
-    case CALLBACKS:
-    case HANDLER:
-    case INTERCEPTOR:
-      return true;
-    case MAP_TRANSITION:
-    case ELEMENTS_TRANSITION:
-    case CONSTANT_TRANSITION:
-    case NULL_DESCRIPTOR:
-      return false;
-  }
-  UNREACHABLE();  // keep the compiler happy
-  return false;
-}
-
-
 // PropertyDetails captures type and attributes for a property.
 // They are used both in property dictionaries and instance descriptors.
 class PropertyDetails BASE_EMBEDDED {

diff --git a/src/property.h b/src/property.h
index d5efb7f35193f9a5ee36dbe9339e968996aaf4d0..9235c3287dc74a302fb61276d3352c6f4587f2d9 100644 (file)
--- a/src/property.h
+++ b/src/property.h
@@ -164,6 +164,35 @@ class CallbacksDescriptor:  public Descriptor {
 };
 
 
+template <class T>
+bool IsPropertyDescriptor(T* desc) {
+  switch (desc->type()) {
+    case NORMAL:
+    case FIELD:
+    case CONSTANT_FUNCTION:
+    case HANDLER:
+    case INTERCEPTOR:
+      return true;
+    case CALLBACKS: {
+      Object* callback_object = desc->GetCallbackObject();
+      // Non-JavaScript (i.e. native) accessors are always a property, otherwise
+      // either the getter or the setter must be an accessor. Put another way:
+      // If we only see map transitions and holes in a pair, this is not a
+      // property.
+      return (!callback_object->IsAccessorPair() ||
+              AccessorPair::cast(callback_object)->ContainsAccessor());
+    }
+    case MAP_TRANSITION:
+    case ELEMENTS_TRANSITION:
+    case CONSTANT_TRANSITION:
+    case NULL_DESCRIPTOR:
+      return false;
+  }
+  UNREACHABLE();  // keep the compiler happy
+  return false;
+}
+
+
 class LookupResult BASE_EMBEDDED {
  public:
   explicit LookupResult(Isolate* isolate)
@@ -261,10 +290,9 @@ class LookupResult BASE_EMBEDDED {
   bool IsFound() { return lookup_type_ != NOT_FOUND; }
   bool IsHandler() { return lookup_type_ == HANDLER_TYPE; }
 
-  // Is the result is a property excluding transitions and the null
-  // descriptor?
+  // Is the result is a property excluding transitions and the null descriptor?
   bool IsProperty() {
-    return IsFound() && IsRealProperty(GetPropertyDetails().type());
+    return IsFound() && IsPropertyDescriptor(this);
   }
 
   bool IsCacheable() { return cacheable_; }