[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 #38 from Diego Pino <dpino at igalia.com>  2014-09-12 12:19:26 PST ---
(From update of attachment 237954)
View in context: https://bugs.webkit.org/attachment.cgi?id=237954&action=review

>> Source/JavaScriptCore/runtime/StringPrototype.cpp:1561
>> +        return throwVMTypeError(exec);
> 
> Why is this needed? I don’t see this in the existing string functions. What test covers this?

It's required by the ES6 spec. It's the first step in 'startsWith' (in addition to 'endsWith' and 'contains').

1. Let O be RequireObjectCoercible(this value).

Other String functions (trim, toLowerCase, substring, etc) require this step too, and it's implemented in the same fashion.

According to the spec, the abstract operation RequireObjectCoercible throws an TypeError Exception if argument cannot be converted to an object using ToObject. The tests that call 'startsWith', 'endsWith' and 'contains' with an environment record 'this', cover this case, they all throw an Exception. More info in comment #9.

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

The logic of the functions that use this 'equalInner' implementation, 'startsWith' and 'endsWith', is rather small, the real matching is done here. If inlining is not going to save much I can't remove it though. FWIW, the other 'equalInner' implementation is inlined.

> Source/WTF/wtf/text/StringImpl.cpp:1379
> +{

As this method is new, I tried to make it as "StringImpl& stringImpl" and pass *this as parameter, but I got the following error:

../../Source/WTF/wtf/text/StringImpl.cpp: In member function 'bool WTF::StringImpl::startsWith(WTF::StringImpl&, unsigned int, bool) const':
../../Source/WTF/wtf/text/StringImpl.cpp:1434:69: error: no matching function for call to 'equalInner(const WTF::StringImpl&, unsigned int&, WTF::StringImpl&, bool&)'
../../Source/WTF/wtf/text/StringImpl.cpp:1434:69: note: candidates are:
../../Source/WTF/wtf/text/StringImpl.cpp:1362:55: note: bool WTF::equalInner(const WTF::StringImpl*, unsigned int, const char*, unsigned int, bool)
../../Source/WTF/wtf/text/StringImpl.cpp:1362:55: note:   candidate expects 5 arguments, 4 provided
../../Source/WTF/wtf/text/StringImpl.cpp:1378:55: note: bool WTF::equalInner(WTF::StringImpl&, unsigned int, WTF::StringImpl&, bool)
../../Source/WTF/wtf/text/StringImpl.cpp:1378:55: note:   no known conversion for argument 1 from 'const WTF::StringImpl' to 'WTF::StringImpl&'

It seems 'const' was required, but Darin told me all StringImpl& are implicitly const. Finally I decide to leave it as "const StringImpl* stringImpl" and pass 'this'.

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

I agree, I don't like it either, but as I commented in #25 this is necessary to avoid ambiguous warning messages from the compiler. If I put 'startOffset' before 'caseSensitive', all the existing code of the form "contains(str)" or "contains(str, false)" will turn to be into "contains(str, 1, true)" and "contains(str, 0, true)", because the compiler does an implicit conversion between boolean types and integers.

At this point I think it might be better to not overload 'contains', 'startsWith' and 'endsWith' methods and name the new ones as 'containsFromPos', 'startsWithFromPos' and 'endsWithUntilPos'. In addition, having a single contains(str, offset, caseSenstive), forces to explicitly set an offset when searching a string non-sensisite, I mean, str.contains(substr, false) must be written as str.contains(substr, 0, false), which is unclear IMHO. I think having two different names helps to avoid this problem.

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