[webkit-reviews] review denied: [Bug 131704] Simple ES6 feature:String prototype additions : [Attachment 237954] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Diego Pino <dpino at igalia.com>'s
request for review:
Bug 131704: Simple ES6 feature:String prototype additions
https://bugs.webkit.org/show_bug.cgi?id=131704

Attachment 237954: Patch
https://bugs.webkit.org/attachment.cgi?id=237954&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.