[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 #237954|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #37 from Darin Adler <darin at apple.com>  2014-09-11 20:19:41 PST ---
(From update of attachment 237954)
View in context: https://bugs.webkit.org/attachment.cgi?id=237954&action=review

Looking better, but still not quite right. Thanks for so much hard work on this!

> Source/JavaScriptCore/runtime/StringPrototype.cpp:88
> +

Shouldn’t add an extra blank line here.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1561
> +    if (!checkObjectCoercible(thisValue))
> +        return throwVMTypeError(exec);

Why is this needed? I don’t see this in the existing string functions. What test covers this?

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1571
> +    unsigned start = std::max(0, exec->argument(1).toInt32(exec));

Need an if (exec->hadException()) here since toInt32 can raise an exception.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1575
> +    if (exec->hadException())
> +        return JSValue::encode(jsUndefined());

As I mentioned in earlier versions of this patch, we don’t actually need this exec->hadException check because it’s the last and no JavaScript executes after this point. If you try to construct a test case that shows this is needed, you will find that it’s impossible. It’s OK to leave it in, but it’s untestable.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1584
> +    if (!checkObjectCoercible(thisValue))
> +        return throwVMTypeError(exec);

Why is this needed? I don’t see this in the existing string functions. What test covers this?

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1596
> +    int pos = a1.isUndefined() ? length : a1.toInt32(exec);

Need an if (exec->hadException()) here since toInt32 can raise an exception.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1601
> +    if (exec->hadException())
> +        return JSValue::encode(jsUndefined());

As I mentioned in earlier versions of this patch, we don’t actually need this exec->hadException check because it’s the last and no JavaScript executes after this point. If you try to construct a test case that shows this is needed, you will find that it’s impossible. It’s OK to leave it in, but it’s untestable.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1610
> +    if (!checkObjectCoercible(thisValue))
> +        return throwVMTypeError(exec);

Why is this needed? I don’t see this in the existing string functions. What test covers this?

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1620
> +    unsigned start = std::max(0, exec->argument(1).toInt32(exec));

Need an if (exec->hadException()) here since toInt32 can raise an exception.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1624
> +    if (exec->hadException())
> +        return JSValue::encode(jsUndefined());

As I mentioned in earlier versions of this patch, we don’t actually need this exec->hadException check because it’s the last and no JavaScript executes after this point. If you try to construct a test case that shows this is needed, you will find that it’s impossible. It’s OK to leave it in, but it’s untestable.

> Source/WTF/wtf/text/StringImpl.cpp:1172
> +    ASSERT(index <= length());
> +    ASSERT(matchLength <= length());
> +    ASSERT(index + matchLength <= length());
> +
> +    if (matchLength + index > length())
> +        return notFound;

All of this is wrong. These cases are handled below. Please don’t add this code.

> Source/WTF/wtf/text/StringImpl.cpp:1378
> +ALWAYS_INLINE static bool equalInner(const StringImpl* stringImpl, unsigned startOffset, StringImpl& matchString, bool caseSensitive)

Do we really need ALWAYS_INLINE for this?

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

This indicates the function should take a StringImpl&, not a StringImpl*. If we can’t take a null we should take a reference, not a pointer.

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

This fourth check is not needed. The three checks above suffice cover this already.

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

This isn’t needed, since we already have this check in equalInner.

> Source/WTF/wtf/text/WTFString.h:262
> +    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; }

This is really not good. We don’t want a startOffset argument that comes after the case sensitive boolean in this function. Is there some way to avoid introducing this?

> Source/WTF/wtf/text/WTFString.h:274
> +    bool startsWith(String& s, unsigned startOffset, bool caseSensitive) const
> +        { return m_impl ? m_impl->startsWith(*s.impl(), startOffset, caseSensitive) : false; }

Please name the argument “prefix”, not “s”. We prefer words to letters for argument names.

It’s not appropriate to assume that s.impl() is non-null without checking. We should not make functions where it’s illegal to call them with null strings.

> Source/WTF/wtf/text/WTFString.h:285
> +    bool endsWith(String& s, unsigned endOffset, bool caseSensitive) const
> +        { return m_impl ? m_impl->endsWith(*s.impl(), endOffset, caseSensitive) : false; }

Please name the argument “suffix”, not “s”. We prefer words to letters for argument names.

It’s not appropriate to assume that s.impl() is non-null without checking. We should not make functions where it’s illegal to call them with null strings.

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