[webkit-reviews] review granted: [Bug 70364] Switched ropes from malloc memory to GC memory : [Attachment 111507] Patch

[bugzilla-daemon at webkit.org]

Gavin Barraclough <barraclough at apple.com> has granted Geoffrey Garen
<ggaren at apple.com>'s request for review:
Bug 70364: Switched ropes from malloc memory to GC memory
https://bugs.webkit.org/show_bug.cgi?id=70364

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

------- Additional Comments from Gavin Barraclough <barraclough at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=111507&action=review


Hmmm, I wonder whether we still need m_fiberCount, if a JSString is a rope, is
m_value null? (to remove the test of m_fiberCount in isRope)
Still, that's a separate question really.

After this patch, will we ever be creating RopeImpls?
Should this patch not be removing RopeImpl.cpp/.h from svn?

r+, but I think you should probably be using jsNontrivialString in the case I
point out, and probably remove the RopeImpl files.

> Source/JavaScriptCore/runtime/JSValue.cpp:222
> +	   return JSString::create(exec->globalData(),
UString("undefined").impl());

Maybe we should have an JSString::create(ExecState*, const char*) helper? - 
Actually, I think we have one! - this should call:

inline JSString* jsNontrivialString(JSGlobalData* globalData, const char* s)

> Source/JavaScriptCore/runtime/Operations.h:96
>      ALWAYS_INLINE JSValue jsString(ExecState* exec, JSValue thisValue)

This method is really poorly named (my bad, I believe :-( ) - there is nothing
in the name indicating it does anything to do with arguments, we should really
rename it to something more explicit.

> Source/JavaScriptCore/runtime/SmallStrings.cpp:109
> +    m_emptyString = JSString::createHasOtherOwner(*globalData,
UString("").impl());

This could be 'StringImpl::empty()'

> Source/JavaScriptCore/runtime/StringPrototype.cpp:640
> +	       : jsString(exec, asString(thisValue),
JSString::create(exec->globalData(), v.toString(exec).impl())));

We create JSStrings with arguments like this in quite a few places, maybe we
should have an JSString::create(ExecState*, JSValue)?