[webkit-reviews] review granted: [Bug 13021] XPath can be very slow : [Attachment 13814] partial fix 3

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alexey Proskuryakov <ap at webkit.org>'s
request for review:
Bug 13021: XPath can be very slow
http://bugs.webkit.org/show_bug.cgi?id=13021

Attachment 13814: partial fix 3
http://bugs.webkit.org/attachment.cgi?id=13814&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
+    Vector<UChar, 1024> result;
+    [...]
+    return String(result.data(), result.size());

I think it might be a slightly better idiom here is to use a plain old
Vector<UChar>, then use String::adopt for the return value. Not sure. There
will be slightly more allocation while accumulating the vector, but saves one
allocation at the resturn site.

-    return nodes;
+    return v;

I don't understand that change; can't we still return nodes?

+	     // Steals nodes from value.
+	     Value(NodeSet& value) : m_type(NodeSetValue), m_data(new
ValueData) { value.swap(m_data->m_nodeSet); }

I think maybe this is a little to dangerous to be a public constructor. Can you
instead make it a private constructor and use a named construction function to
expose this? Or maybe a clearer way would be to just construct an empty Value
and then call a function named "adopt" that takes a NodeSet&. At the very
least, I think this should be labeled "explicit".

Great changes, r=me