Bug 115589 – Move IdentifierASCIIStringTranslator to WTF

• Status: NEW
• Component: JavaScriptCore

[Xan Lopez]

This removes a layering violation. This translator is very similar to the CString one, but the translate method is different and I think the difference is relevant (with the ASCII one creating the StringImpl through the createFromLiteral ctor), so I'm leaving things as they are for now.

[Xan Lopez]

Created attachment 200531 [details]
asciistringtranslator.diff

[Xan Lopez]

Created attachment 200533 [details]
asciistringtranslator.diff

Rebase against trunk.

[Benjamin Poulain]

Comment on attachment 200533 [details]
asciistringtranslator.diff

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.

[Xan Lopez]

So exposing StringImpl::setHash would obviously fix all layering violations here, and we'd done. I assumed there were good reasons to avoid that, but if the are not we can just do it.

Otherwise I guess I can try to figure out how to generalize all this stuff in a StringHashTranslator class as you suggest in bug #115593.

[Benjamin Poulain]

(In reply to comment #4)
> So exposing StringImpl::setHash would obviously fix all layering violations here, and we'd done. I assumed there were good reasons to avoid that, but if the are not we can just do it.
> 
> Otherwise I guess I can try to figure out how to generalize all this stuff in a StringHashTranslator class as you suggest in bug #115593.

setHash is private in order to be defensive since only a a few classes should do that (mostly Translators and AtomicString).

I guess we could have on abstract StringHash that is a friend of StringImpl and with a protected method to set the hash. What do you think?