[webkit-reviews] review denied: [Bug 115589] Move IdentifierASCIIStringTranslator to WTF : [Attachment 200533] asciistringtranslator.diff

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Xan Lopez
<xan.lopez at gmail.com>'s request for review:
Bug 115589: Move IdentifierASCIIStringTranslator to WTF
https://bugs.webkit.org/show_bug.cgi?id=115589

Attachment 200533: asciistringtranslator.diff
https://bugs.webkit.org/attachment.cgi?id=200533&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=200533&action=review


This should be defined in StringHash.h, not AtomicString.h.

The name should be ASCIILiteralToStringTranslator.

I am not sure it is a good change. I like the idea of generalizing the
translator. On the other side, it is not good to have uncommon concepts far
from where they are used.
An other option is to expose StringImpl::setHash().

> Source/JavaScriptCore/runtime/Identifier.cpp:87
> +    HashSet<StringImpl*>::AddResult addResult = identifierTable.add<const
LChar*, WTF::ASCIIStringTranslator>(reinterpret_cast<const LChar*>(c));

Instead of prefixing with WTF, you should expose it like the other ADT of WTF.

IMHO, the translator should take a char* instead of LChar.

> Source/WTF/wtf/text/AtomicString.h:263
> +// This needs to be in the header since it's used by JSC::Identifier

This comment is a false good idea.
JSC::Identifier will evolve independently from this translator.