[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