jsSubstring() should support creating substrings from substrings.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Andreas Kling]

It would be neat if jsSubstring() would be smarter when asked to create a substring of a JSString that is itself a substring.
It should share the base string and just modify the offset instead.

[Andreas Kling]

Created attachment 253831 [details]
Patch

[Geoffrey Garen]

Comment on attachment 253831 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=253831&action=review

Yo dawg.

> Source/JavaScriptCore/runtime/JSString.h:532
> +    if (s->isRope() && !static_cast<JSRopeString*>(s)->isSubstring())
> +        static_cast<JSRopeString*>(s)->resolveRope(exec);

Can we move this branch into the callee?

It feels unsafe that the callee assumes it will not be passed a non-substring rope, when nothing in its type signature guarantees it.

[Darin Adler]

Comment on attachment 253831 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=253831&action=review

> Source/JavaScriptCore/runtime/JSString.h:304
> +            JSRopeString* baseRope = static_cast<JSRopeString*>(base);

I suggest we use a reference rather than pointer here.

>> Source/JavaScriptCore/runtime/JSString.h:532
>> +        static_cast<JSRopeString*>(s)->resolveRope(exec);
> 
> Can we move this branch into the callee?
> 
> It feels unsafe that the callee assumes it will not be passed a non-substring rope, when nothing in its type signature guarantees it.

I agree that would be better.

[Andreas Kling]

Created attachment 254629 [details]
Patch

Take 2, rope resolution moved to callee as requested by reviewers.

[Geoffrey Garen]

Comment on attachment 254629 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254629&action=review

r=me

> Source/JavaScriptCore/runtime/JSString.h:314
> +        // For now, let's not allow substrings with a rope base.
> +        // Resolve non-substring rope bases so we don't have to deal with it.

Might be nice to elaborate on what we're trying to avoid.

> Source/JavaScriptCore/runtime/JSString.h:315
> +        if (base.isRope() && !base.isSubstring())

Is it possible for our base to be a substring? I think it is not, since all substrings will do the base forwarding above upon creation.

[Andreas Kling]

Landed in r185486