Better implementation of DOMPropertyAccessorMap

Summary: Base the property order on the input table, use a set instead of a map, and get rid of the duplicate map. Reviewed By: @bertmaher Differential Revision: D1712866 Signature: t1:1712866:1417547052:aca10c82ce8595f72c4aff549630163e0872b6bf
facebook · Dec 3, 2014 · c3d5fb8 · c3d5fb8
1 parent 510138a
commit c3d5fb8
Show file tree

Hide file tree

Showing 9 changed files with 456 additions and 446 deletions.
diff --git a/hphp/runtime/ext/domdocument/ext_domdocument.cpp b/hphp/runtime/ext/domdocument/ext_domdocument.cpp
@@ -1527,85 +1527,95 @@ struct DOMPropertyAccessor {
 
 const StaticString s_object_value_omitted("(object value omitted)");
 
+struct hashdpa {
+  size_t operator()(const DOMPropertyAccessor* da) const {
+    return hash_string_i(da->name, strlen(da->name));
+  }
+};
+struct cmpdpa {
+  bool operator()(const DOMPropertyAccessor* da1,
+                  const DOMPropertyAccessor* da2) const {
+    return strcasecmp(da1->name, da2->name) == 0;
+  }
+};
+
 class DOMPropertyAccessorMap :
-      private hphp_const_char_map<DOMPropertyAccessor*> {
+      private hphp_hash_set<DOMPropertyAccessor*, hashdpa, cmpdpa> {
 public:
   explicit DOMPropertyAccessorMap(DOMPropertyAccessor* props,
                                   DOMPropertyAccessorMap *base = nullptr) {
     if (base) {
       *this = *base;
     }
     for (DOMPropertyAccessor *p = props; p->name; p++) {
-      (*this)[p->name] = p;
-      m_imap[p->name] = p;
+      this->insert(p);
+      m_props.push_back(p);
     }
   }
 
   Variant (*getter(const Variant& name))(const Object&) {
     if (name.isString()) {
-      const char* name_data = name.toString().data();
-      const_iterator iter = find(name_data);
-      const_iterator iiter = m_imap.find(name_data);
-      if (iter != end() && iter->second->getter) {
-        return iter->second->getter;
-      } else if (iiter != end() && iiter->second->getter) {
-        raise_warning("Accessing DOMNode derived property '%s' with the "
-                      "incorrect casing", name_data);
-        return iiter->second->getter;
+      auto dpa = DOMPropertyAccessor {
+        name.toString().data(), nullptr, nullptr
+      };
+      const_iterator iter = find(&dpa);
+      if (iter != end() && (*iter)->getter) {
+        if (strcmp(dpa.name, (*iter)->name)) {
+          raise_warning("Accessing DOMNode derived property '%s' with the "
+                        "incorrect casing", dpa.name);
+        }
+        return (*iter)->getter;
       }
     }
     return dummy_getter;
   }
 
   void (*setter(const Variant& name))(const Object&, const Variant&) {
     if (name.isString()) {
-      const char* name_data = name.toString().data();
-      const_iterator iter = find(name_data);
-      const_iterator iiter = m_imap.find(name_data);
-      if (iter != end() && iter->second->setter) {
-        return iter->second->setter;
-      } else if (iiter != end() && iiter->second->setter) {
-        raise_warning("Setting DOMNode derived property '%s' with the "
-                      "incorrect casing", name_data);
-        return iiter->second->setter;
+      auto dpa = DOMPropertyAccessor {
+        name.toString().data(), nullptr, nullptr
+      };
+      const_iterator iter = find(&dpa);
+      if (iter != end() && (*iter)->setter) {
+        if (strcmp(dpa.name, (*iter)->name)) {
+          raise_warning("Setting DOMNode derived property '%s' with the "
+                        "incorrect casing", dpa.name);
+        }
+        return (*iter)->setter;
       }
     }
     return dummy_setter;
   }
 
   bool isset(ObjectData *obj, const String& name) {
-    const_iterator iter = find(name.data());
-    const_iterator iiter = m_imap.find(name.data());
-    if (iter == end() && iiter == m_imap.end()) {
-      return false;
-    } else if (iter != end()) {
-      return !iter->second->getter(obj).isNull();
-    } else {
-      raise_warning("Accessing DOMNode derived property '%s' with the "
-                    "incorrect casing", name.data());
-      return !iiter->second->getter(obj).isNull();
+    auto dpa = DOMPropertyAccessor {
+      name.data(), nullptr, nullptr
+    };
+    const_iterator iter = find(&dpa);
+    if (iter != end() && (*iter)->getter) {
+      if (strcmp(dpa.name, (*iter)->name)) {
+        raise_warning("Accessing DOMNode derived property '%s' with the "
+                      "incorrect casing", dpa.name);
+      }
+      return !(*iter)->getter(obj).isNull();
     }
+    return false;
   }
 
   Array debugInfo(ObjectData* obj) {
     Array ret = obj->o_toArray();
-    for (auto it : *this) {
-      auto value = it.second->getter(obj);
+    for (auto it : m_props) {
+      auto value = it->getter(obj);
       if (value.isObject()) {
         value = s_object_value_omitted;
       }
-      ret.set(String(it.first, CopyString), value);
+      ret.set(String(it->name, CopyString), value);
     }
     return ret;
   }
 
 private:
-  // Previously, this class was backed by an imap. This led to a lot of
-  // code relying on accessing properties that were improperly cased.
-  // Since removing this functionality could cause a lot of functionality
-  // to break, instead we continue to allow access case-insensitively, but
-  // with a warning
-  hphp_const_char_imap<DOMPropertyAccessor*> m_imap;
+  std::vector<DOMPropertyAccessor*> m_props;
 };
 
 ///////////////////////////////////////////////////////////////////////////////

diff --git a/hphp/test/quick/builtin_extension_DOMComment.php.expectf b/hphp/test/quick/builtin_extension_DOMComment.php.expectf
@@ -1,41 +1,41 @@
 DOMComment
 object(DOMComment)#1 (18) {
-  ["length"]=>
-  int(0)
-  ["textContent"]=>
-  string(0) ""
-  ["baseURI"]=>
-  NULL
-  ["localName"]=>
-  NULL
-  ["prefix"]=>
+  ["nodeName"]=>
+  string(8) "#comment"
+  ["nodeValue"]=>
   string(0) ""
-  ["ownerDocument"]=>
-  NULL
+  ["nodeType"]=>
+  int(8)
   ["parentNode"]=>
   NULL
-  ["namespaceURI"]=>
+  ["childNodes"]=>
+  NULL
+  ["firstChild"]=>
   NULL
-  ["nodeType"]=>
-  int(8)
   ["lastChild"]=>
   NULL
-  ["nodeName"]=>
-  string(8) "#comment"
   ["previousSibling"]=>
   NULL
-  ["firstChild"]=>
+  ["nextSibling"]=>
   NULL
-  ["data"]=>
-  string(0) ""
-  ["childNodes"]=>
+  ["attributes"]=>
   NULL
-  ["nodeValue"]=>
+  ["ownerDocument"]=>
+  NULL
+  ["namespaceURI"]=>
+  NULL
+  ["prefix"]=>
   string(0) ""
-  ["nextSibling"]=>
+  ["localName"]=>
   NULL
-  ["attributes"]=>
+  ["baseURI"]=>
   NULL
+  ["textContent"]=>
+  string(0) ""
+  ["data"]=>
+  string(0) ""
+  ["length"]=>
+  int(0)
 }
 
 Warning: Attempted to serialize unserializable builtin class DOMComment in %s/test/quick/builtin_extensions.inc on line 8
@@ -102,42 +102,42 @@ A_DOMComment
 object(A_DOMComment)#2 (19) {
   ["___x"]=>
   NULL
-  ["length"]=>
-  int(0)
-  ["textContent"]=>
-  string(0) ""
-  ["baseURI"]=>
-  NULL
-  ["localName"]=>
-  NULL
-  ["prefix"]=>
+  ["nodeName"]=>
+  string(8) "#comment"
+  ["nodeValue"]=>
   string(0) ""
-  ["ownerDocument"]=>
-  NULL
+  ["nodeType"]=>
+  int(8)
   ["parentNode"]=>
   NULL
-  ["namespaceURI"]=>
+  ["childNodes"]=>
+  NULL
+  ["firstChild"]=>
   NULL
-  ["nodeType"]=>
-  int(8)
   ["lastChild"]=>
   NULL
-  ["nodeName"]=>
-  string(8) "#comment"
   ["previousSibling"]=>
   NULL
-  ["firstChild"]=>
+  ["nextSibling"]=>
   NULL
-  ["data"]=>
-  string(0) ""
-  ["childNodes"]=>
+  ["attributes"]=>
   NULL
-  ["nodeValue"]=>
+  ["ownerDocument"]=>
+  NULL
+  ["namespaceURI"]=>
+  NULL
+  ["prefix"]=>
   string(0) ""
-  ["nextSibling"]=>
+  ["localName"]=>
   NULL
-  ["attributes"]=>
+  ["baseURI"]=>
   NULL
+  ["textContent"]=>
+  string(0) ""
+  ["data"]=>
+  string(0) ""
+  ["length"]=>
+  int(0)
 }
 
 Warning: Attempted to serialize unserializable builtin class A_DOMComment in %s/test/quick/builtin_extensions.inc on line 26