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

[bugzilla-daemon at webkit.org]

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





--- Comment #22 from Diego Pino <dpino at igalia.com>  2014-08-25 15:41:03 PST ---
(From update of attachment 237082)
View in context: https://bugs.webkit.org/attachment.cgi?id=237082&action=review

>> 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()).

Thanks for the extensive review.

Originally I did that but Geoffrey suggested me to use jsDynamicCast instead. See comment #9.

>> 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.

OK

>> 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.

OK. The variable is used in line 1578, although could be replaced directly as it's the only line where is used.

> uint32_t start = std::min(static_cast<uint32_t>(std::max(pos, 0)), len);

>> 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”.

What about "matchString"? It is what StringImpl uses and it will be consistent. If not, "stringToSearchFor" sounds good to me.

>> Source/JavaScriptCore/runtime/StringPrototype.cpp:1576
>> +    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));

OK.

>> 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.

OK.

>> 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.

OK.

>> 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.

OK, agree. My doubt is that many other methods in this file (the several implementations of startsWith and endsWith, for instance) use "const StringImpl*" as datatype and follow the same naming convention for "stringImpl". I think that maybe is valuable to use the same datatype and naming convention for keeping consistency, if not I can change it. WDYT?

>> 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.

OK.

>> 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.

OK.

>> Source/WTF/wtf/text/StringImpl.cpp:1384
>> +            memcpy((LChar*) str, (LChar*) str, endOffset);
> 
> This code is nonsense and does nothing. What were we trying to do here?

What I was trying to do was to shrink "str", being its new length "endOffset" characters. But yes, you are correct, this code has no effect. So I was wondering how is that this still working? Well, the reason is that the value of "endOffset" is implicitly set in "startOffset". In StringImpl:endsWith:

> int startOffset = endOffset - matchString->length();
> if (startOffset < 0)
>    return false;

> return equalInner(this, startOffset, endOffset, matchString, caseSensitive);

So basically the parameter "endOffset" in this method is useless. There's another "equalInner" implementation in this class. It doesn't has a "endOffset" parameter and "matchString" is "const char*" instead of "StringImpl *". I guess I will use this method instead, and remove the one I added.

>> Source/WTF/wtf/text/StringImpl.cpp:1431
>> +        return false;
> 
> This can overflow. It needs to be rewritten in a way that cannot overflow.

OK.

>> Source/WTF/wtf/text/StringImpl.cpp:1461
>> +    ASSERT(matchLength);
> 
> How does this compile? There is nothing named matchLength.

True. AFAIK, ASSERTs are removed in shipping builds, it could be that. I added a new ASSERT with a non-existent variable and compiled :/ In any case, I'd expect this failed.

>> 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*.

OK.

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

OK.

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

OK.

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

OK.

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