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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Aug 25 15:55:12 PDT 2014


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





--- Comment #23 from Darin Adler <darin at apple.com>  2014-08-25 15:55:16 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.

OK, Geoff is the authority on this, so go with what he said.

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

Either of those is OK with me.

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

Using & instead of * for things that are never null is newly the style in WebKit this year, so older code would be using * and newer &. As far as const, it’s an easy mistake to make. Someone might not realize that StringImpl is an immutable class.

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

The idiom here for not overflowing is:

    ASSERT(startOffset <= stringImpl->length());
    ASSERT(matchString->length() <= stringImpl->length());
    ASSERT(startOffset + matchString->length() <= stringImpl->length());

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


More information about the webkit-unassigned mailing list