[Webkit-unassigned] [Bug 115589] Move IdentifierASCIIStringTranslator to WTF

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=115589


Benjamin Poulain <benjamin at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #200533|review?                     |review-
               Flag|                            |




--- Comment #3 from Benjamin Poulain <benjamin at webkit.org>  2013-05-04 16:45:32 PST ---
(From update of attachment 200533)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.