Bug 115589 - Move IdentifierASCIIStringTranslator to WTF
Summary: Move IdentifierASCIIStringTranslator to WTF
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-04 09:43 PDT by Xan Lopez
Modified: 2013-05-04 21:17 PDT (History)
4 users (show)

See Also:


Attachments
asciistringtranslator.diff (6.88 KB, patch)
2013-05-04 09:46 PDT, Xan Lopez
no flags Details | Formatted Diff | Diff
asciistringtranslator.diff (6.80 KB, patch)
2013-05-04 09:57 PDT, Xan Lopez
benjamin: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xan Lopez 2013-05-04 09:43:25 PDT
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.
Comment 1 Xan Lopez 2013-05-04 09:46:17 PDT
Created attachment 200531 [details]
asciistringtranslator.diff
Comment 2 Xan Lopez 2013-05-04 09:57:19 PDT
Created attachment 200533 [details]
asciistringtranslator.diff

Rebase against trunk.
Comment 3 Benjamin Poulain 2013-05-04 16:45:32 PDT
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.
Comment 4 Xan Lopez 2013-05-04 20:47:45 PDT
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.
Comment 5 Benjamin Poulain 2013-05-04 21:17:27 PDT
(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?