[webkit-reviews] review granted: [Bug 112831] HTMLNames should construct strings at compile time : [Attachment 194104] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Adam Barth <abarth at webkit.org>'s
request for review:
Bug 112831: HTMLNames should construct strings at compile time
https://bugs.webkit.org/show_bug.cgi?id=112831

Attachment 194104: Patch
https://bugs.webkit.org/attachment.cgi?id=194104&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=194104&action=review


Seems good. Anders and Ben were talking about making a version of the hash
function that runs at C++ compile time, but given we already have a script in
play here, seems good to do it this way.

> Source/WTF/wtf/text/StringImpl.h:777
> +    struct StaticData {

Looks good. I’d probably name this something more like StaticASCIILiteral, to
make it clearer why it’s OK to just have m_data and why it’s OK to have initial
values that mimic ConstructFromLiteral.

> Source/WTF/wtf/text/StringImpl.h:782
> +	   unsigned m_refCount;
> +	   unsigned m_length;
> +	   const LChar* m_data8;
> +	   void* m_buffer;
> +	   unsigned m_hashAndFlags;

Normally for a struct we’d leave off the m_ prefixes, but I can see an argument
for keeping them here. Might be clearer with a comment stating that these match
the data members of the StringImpl class itself.

> Source/WTF/wtf/text/StringImpl.h:791
> +    void assertValidHash()

How about the name assertHasValidHash?

Since these are assertions they don’t need to be inlined, why not put it the
code the .cpp file instead of the .h file?

> Source/WTF/wtf/text/StringImpl.h:798
> +private:

I suggest putting a comment here about how this data needs to match the struct
above.

> Source/WebCore/dom/QualifiedName.cpp:177
> +    new (reinterpret_cast<void*>(targetAddress)) QualifiedName(nullAtom,
AtomicString(name), nameNamespace);

Not new to your patch, but why do we reinterpret_cast a void* to void*?

> Source/WebCore/dom/QualifiedName.h:148
> -void createQualifiedName(void* targetAddress, const char* name, unsigned
nameLength);
> -void createQualifiedName(void* targetAddress, const char* name, unsigned
nameLength, const AtomicString& nameNamespace);
> +void createQualifiedName(void* targetAddress, StringImpl* name);
> +void createQualifiedName(void* targetAddress, StringImpl* name, const
AtomicString& nameNamespace);

Would be nice if it was clearer that these are for some kind of exotic use.
Might even want to use the word construct instead of create to emphasize that.

> Source/WebCore/dom/make_names.pl:132
> +    print F "    // Note: These AtomicStrings might end up with different
StringImpls,\n";
> +    print F "    // but they will always end up with static StringImpls.\n";


This comment seems a bit enigmatic. I think I’m an expert on AtomicString and
I’m not exactly sure what it means or why it’s true. What does it mean to say
they always “end up with static impls”? And is the reason they do because of
the timing of when this function is called? Or some other reason?