[Webkit-unassigned] [Bug 155795] [JSC] implement String.prototype.padStart() and String.prototype.padEnd() proposal

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=155795

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #274829|review?                     |review+
              Flags|                            |

--- Comment #10 from Darin Adler <darin at apple.com> ---
Comment on attachment 274829
  --> https://bugs.webkit.org/attachment.cgi?id=274829
Patch

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

> Source/JavaScriptCore/runtime/StringPrototype.cpp:829
> +enum StringPaddingMode {

In newer code we often use an enum class.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:831
> +    kPadStart,
> +    kPadEnd

The style guide for WebKit suggests we not use a “k” prefix for this type of thing.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:832
> +};

Also, why call it “mode”? Why not just call it the padding location, since it’s the location of the padding?

    enum class StringPaddingLocation { Start, End };

> Source/JavaScriptCore/runtime/StringPrototype.cpp:834
> +template <StringPaddingMode mode>

I think we can afford the one to three branches it would cost to have this be a function with an argument rather than a template. I’m not sure it’s worth having an entire separate copy of this function just to avoid a small number of branches in a function that also does some memory allocation, much more expensive than branches. Please consider using a function rather than a function template.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:835
> +static EncodedJSValue stringPadHelper(ExecState* exec)

I suggest naming this padString or padStartOrEnd, rather than "stringPadHelper". We normally use verbs for function names.

In new code, I suggest considering use of "ExecState& state" rather than "ExecState* exec" since it can never be null.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:842
> +    JSString* thisString = thisValue.toString(exec);
> +    if (exec->hadException())
> +        return JSValue::encode(jsUndefined());

Non-obvious, but the more efficient idiom for this in new code is:

    JSString* thisString = thisValue.toStringOrNull(exec);
    if (!thisString)
        return JSValue::encode(jsUndefined());

> Source/JavaScriptCore/runtime/StringPrototype.cpp:847
> +    RELEASE_ASSERT(maxLengthAsDouble >= 0.0 && maxLengthAsDouble == std::trunc(maxLengthAsDouble));

It’s wasteful to do a RELEASE_ASSERT for something that is both guaranteed to be true, and also would also cause no real trouble if it was false, since there is range checking code below that can deal with it perfectly (as long as we don’t unnecessarily convert to uint64_t first).

If you feel the need to assert, then ASSERT, by all means, but I don’t think this is an important thing to assert since it’s an invariant of the result of the toLength function.

With ASSERT, we almost never use &&, because we’d rather use two separate ASSERT. No reason to be unnecessarily vague about which of the two expressions was false, which is what happens when we use &&.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:848
> +    uint64_t maxLength = static_cast<uint64_t>(maxLengthAsDouble);

I do not think it is valuable to convert to a 64-bit integer to do the range check. Instead I suggest we do the range check with the double, and then convert directly from double to unsigned instead of involving uint64_t at all.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:854
> +    // Ensure that the resulting string's length is in the range of int32_t
> +    if (maxLength > std::numeric_limits<int32_t>::max())

Sure would be nice if this limit and other checks like it were based on a maximum JavaScript string length constant rather than explicitly based on int32_t’s range; it’s annoying to have to code in the implicit knowledge that this is the maximum JavaScript string length.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:860
> +        filler = jsSingleCharacterString(exec, ' ');

Maybe we should use nullptr to mean "no fill string argument" and convert that to a space below in the single character special case, instead of actually calling jsSingleCharacterString. A shame to waste any time looking up the JSString for space when all we are going to do is re-extract the first character from it.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:864
> +        filler = fillString.toString(exec);
> +        if (exec->hadException())
> +            return JSValue::encode(jsUndefined());

Non-obvious, but the more efficient idiom for this in new code is:

    filler = fillString.toStringOrNull(exec);
    if (!filler)
        return JSValue::encode(jsUndefined());

> Source/JavaScriptCore/runtime/StringPrototype.cpp:872
> +    if (filler->length() == 1) {
> +        UChar character = filler->value(exec).at(0);

We can use the unsafeView function instead of the value function here to avoid unnecessary string allocation and reference counting. If we combine that with the use of nullptr to mean a space suggested above, this would be:

    if (!filler || filler->length() == 1) {
        UChar character = filler ? filler->unsafeView(exec)[0] : ' ';

> Source/JavaScriptCore/runtime/StringPrototype.cpp:876
> +        if (!(character & ~0xff))
> +            filler = repeatCharacter(exec, static_cast<LChar>(character), fillLength).toString(exec);
> +        else
> +            filler = repeatCharacter(exec, character, fillLength).toString(exec);

It’s wasteful here to call repeatCharacter to make a JSString and convert to a general JSValue, but then call toString to convert back from a JSValue to a JSString*. Instead the repeatCharacter function should be changed to return a JSString*. I think it can return a nullptr when it throws an out of memory exception; although we’d need to be sure to test that doesn’t break stringProtoFuncRepeat.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:878
> +        VM& vm = exec->vm();

Not sure a local variable is best style for this since we are only using it once below.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:890
> +        unsigned repeatCount = fillLength / filler->length();
> +        unsigned remainingCharacters = fillLength % filler->length();
> +        JSRopeString::RopeBuilder ropeBuilder(vm);
> +        for (unsigned i = 0; i < repeatCount; ++i) {
> +            if (!ropeBuilder.append(filler))
> +                return JSValue::encode(throwOutOfMemoryError(exec));
> +        }
> +        if (remainingCharacters) {
> +            if (!ropeBuilder.append(jsSubstring(exec, filler, 0, remainingCharacters)))
> +                return JSValue::encode(throwOutOfMemoryError(exec));
> +        }
> +        filler = ropeBuilder.release();

I think it would be nice to factor this code into a helper function. Might even call it repeatString and have it match the style of repeatCharacter. However, might be better to have it take a RopeBuilder& argument so you can use the same rope to prepend/append thisString to the same rope builder.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:892
> +    ASSERT(!exec->hadException());

Thus assertion will be wrong if repeatCharacter throws an out-of-memory error. We need to include an early exit for that case.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:896
> +    if (mode == kPadEnd)
> +        return JSValue::encode(JSRopeString::create(exec->vm(), thisString, filler));
> +    return JSValue::encode(JSRopeString::create(exec->vm(), filler, thisString));

I think it would be nicer for the non-single-character case to use only a single rope rather than building a first rope and then using that rope to build a second second rope. We’d just append at the start for kPadEnd and at the end for kPadStart. Wasteful to do it in the two separate steps like this.

For the single character case we might still want to keep this code since we won’t already have built a rope.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160325/54f8ccbd/attachment-0001.html>