[webkit-reviews] review denied: [Bug 84907] Memory wasted in JSString for non-rope strings : [Attachment 139058] Another Windows build fix - added new symbol to exports

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Michael Saboff
<msaboff at apple.com>'s request for review:
Bug 84907: Memory wasted in JSString for non-rope strings
https://bugs.webkit.org/show_bug.cgi?id=84907

Attachment 139058: Another Windows build fix - added new symbol to exports
https://bugs.webkit.org/attachment.cgi?id=139058&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=139058&action=review


Looks good, but some small tweaks required before landing.

> Source/JavaScriptCore/runtime/JSString.cpp:63
> +    if (thisObject->isJSRopeStringObject())
> +	   JSRopeString::visitChildren(thisObject, visitor);
> +}
> +
> +void JSRopeString::visitChildren(JSString* thisObject, SlotVisitor& visitor)


It's slightly dirty pool to have two classes with distinct GC visit functions
but identical ClassInfos. ClassInfo is supposed to contain a pointer to an
object's visitChildren function. It's also against our convention to have a
subclass "visitChildren" function that never calls its base class's
"visitChildren" function.

I think a slightly better way to represent this is to fold the rope-style
marking code into JSString::visitChildren. That way, JSString's ClassInfo is
still "true", and JSRopeString is just an implementation detail for the fact
that a JSString may be variable-sized.

> Source/JavaScriptCore/runtime/JSString.cpp:65
> +    JSRopeString* thisObjectAsRope =
reinterpret_cast<JSRopeString*>(thisObject);

static_cast is the C++ cast for casting a base class pointer to a subclass
pointer.

> Source/JavaScriptCore/runtime/JSString.cpp:157
> +	       JSRopeString* currentFiberAsRope =
reinterpret_cast<JSRopeString*>(currentFiber);

static_cast is the C++ cast for casting a base class pointer to a subclass
pointer.

> Source/JavaScriptCore/runtime/JSString.cpp:186
> +	       JSRopeString* currentFiberAsRope =
reinterpret_cast<JSRopeString*>(currentFiber);

static_cast is the C++ cast for casting a base class pointer to a subclass
pointer.

> Source/JavaScriptCore/runtime/JSString.h:167
> +	   bool isRope() const { return m_value.isNull(); }
> +	   bool is8Bit() const { return m_is8Bit; }
> +	   bool isJSRopeStringObject() const { return m_isJSRopeStringObject; }


I'd rather not have two different ways to ask "Am I a rope?" -- It's hard to
tell which one to use when. Instead, please always use the existing isRope()
function, and remove the new one.

> Source/JavaScriptCore/runtime/JSString.h:349
> +	       reinterpret_cast<const JSRopeString*>(this)->resolveRope(exec);

static_cast is the C++ cast for casting a base class pointer to a subclass
pointer.

> Source/JavaScriptCore/runtime/JSString.h:356
> +	       reinterpret_cast<const JSRopeString*>(this)->resolveRope(0);

static_cast is the C++ cast for casting a base class pointer to a subclass
pointer.

> Source/JavaScriptCore/runtime/JSString.h:364
> +	       return
reinterpret_cast<JSRopeString*>(this)->getIndexSlowCase(exec, i);

static_cast is the C++ cast for casting a base class pointer to a subclass
pointer.