[Webkit-unassigned] [Bug 131704] Simple ES6 feature:String prototype additions

[bugzilla-daemon at webkit.org]

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


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #237082|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #21 from Darin Adler <darin at apple.com>  2014-08-25 12:13:46 PST ---
(From update of attachment 237082)
View in context: https://bugs.webkit.org/attachment.cgi?id=237082&action=review

Good approach, but there are some mistakes here, so it’s not ready to go yet.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1564
> +    if (jsDynamicCast<RegExpObject*>(a0))

I believe we should only use jsDynamicCast when we want a pointer. If we are just looking for a boolean predicate, then we should just use a0.inherits(RegExpObject::info()).

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1570
> +    String str = thisValue.toString(exec)->value(exec);

I know the other functions in this file are using this kind of shorthand, but there is little value in using “str” instead of “string” for this local variable, or maybe even stringToSearchIn.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1571
> +    uint32_t len = str.length();

Same thing about “len” vs. “length”. But also, this variable isn’t needed at all.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1573
> +    String searchStr = a0.toString(exec)->value(exec);

And “searchStr” vs. “searchString”. But also, search string could either be the string we are searching "in" or the string we are searching "for". I’d use a name like “targetString” or “stringToSearchFor”.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1576
> +    JSValue a1 = exec->argument(1);
> +    int32_t pos = a1.isUndefined() ? 0 : a1.toInt32(exec);

The check for isUndefined here is unneeded. toInt32 will already yield zero for undefined as well as for null and quite a few other types. The code should just say:

    int position = exec->argument(1).toInt32(exec);

Or even better:

    unsigned start = std::max(0, exec->argument(1).toInt32(exec));

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1578
> +    uint32_t start = std::min(static_cast<uint32_t>(std::max(pos, 0)), len);

This call to std::min is not needed: String::startsWith already will handle that case.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1580
> +    return JSValue::encode(jsBoolean(str.startsWith(searchStr, start, true)));

We should make caseSensitive default to true and not pass it explicitly here. There should be far fewer local variables here.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1583
> +EncodedJSValue JSC_HOST_CALL stringProtoFuncEndsWith(ExecState* exec)

Same comments apply here.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1609
> +EncodedJSValue JSC_HOST_CALL stringProtoFuncContains(ExecState* exec)

Same comments apply here.

> Source/WTF/wtf/text/StringImpl.cpp:1373
> +    ASSERT(stringImpl);

If we are asserting a pointer is not null then it should be a reference, not a pointer. Both stringImpl and matchString should be of type StringImpl&. There is no need to mark it const, because a StringImpl is intrinsically const. The argument names don’t need “impl” in their names.

> Source/WTF/wtf/text/StringImpl.cpp:1375
> +    ASSERT(startOffset + matchString->length() <= stringImpl->length());

This can overflow. It should be written so it can’t overflow.

> Source/WTF/wtf/text/StringImpl.cpp:1379
> +    if (endOffset >= stringImpl->length())
> +        endOffset = stringImpl->length();

It’s strange that this function does this work for endOffset, but not startOffset, where it instead asserts.

> Source/WTF/wtf/text/StringImpl.cpp:1382
> +        const LChar* str = stringImpl->characters8() + startOffset;

Please don’t name a character pointer variable “str”. I think characters is a good name for this.

> Source/WTF/wtf/text/StringImpl.cpp:1384
> +        if (endOffset < stringImpl->length())
> +            memcpy((LChar*) str, (LChar*) str, endOffset);

This code is nonsense and does nothing. What were we trying to do here?

> Source/WTF/wtf/text/StringImpl.cpp:1391
> +    if (endOffset < stringImpl->length())
> +        memcpy((UChar*) str, (UChar*) str, endOffset);

This code is nonsense and does nothing. What were we trying to do here?

> Source/WTF/wtf/text/StringImpl.cpp:1431
> +    if (matchString->length() + startOffset > length())
> +        return false;

This can overflow. It needs to be rewritten in a way that cannot overflow.

> Source/WTF/wtf/text/StringImpl.cpp:1461
> +    ASSERT(matchLength);

How does this compile? There is nothing named matchLength.

> Source/WTF/wtf/text/StringImpl.cpp:1462
> +    int startOffset = endOffset - matchString->length();

This can overflow. It needs to be written in a way that cannot overflow.

> Source/WTF/wtf/text/StringImpl.h:676
> +    WTF_EXPORT_STRING_API bool startsWith(const StringImpl *, unsigned startOffset, bool caseSensitive) const;

Argument type should be StringImpl&, not const StringImpl*.

> Source/WTF/wtf/text/StringImpl.h:683
> +    WTF_EXPORT_STRING_API bool endsWith(const StringImpl *, unsigned endOffset, bool caseSensitive) const;

Argument type should be StringImpl&, not const StringImpl*.

> Source/WTF/wtf/text/WTFString.h:260
> +    bool contains(const LChar* str, bool caseSensitive = true, unsigned startOffset = 0) const { return find(str, startOffset, caseSensitive) != notFound; }
> +    bool contains(const String& str, bool caseSensitive = true, unsigned startOffset = 0) const { return find(str, startOffset, caseSensitive) != notFound; }

We don’t want the case sensitive boolean after the start offset. You should make a separate overload to add the start offset rather than tacking it on the end.

> Source/WTF/wtf/text/WTFString.h:271
> +    bool startsWith(const String& s, unsigned startOffset, bool caseSensitive)

Should default case sensitive to true.

> Source/WTF/wtf/text/WTFString.h:282
> +    bool endsWith(const String& s, unsigned endOffset, bool caseSensitive) const

Should default case sensitive to true.

> Source/WTF/wtf/text/WTFString.h:283
> +    { return m_impl ? m_impl->endsWith(s.impl(), endOffset, caseSensitive) : false; }

Incorrect indentation.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.