[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 #25 from Diego Pino <dpino at igalia.com>  2014-08-26 13:31:57 PST ---
I applied the suggestions pointed out in the last review.

There's a change I was not able to do, which is setting a default value for argument "caseSensitive". The problem is that the signature of the new added methods collide with other existing methods, and although the compiler is able to solve them there are many annoying Warning messages during the build. More precisely, the problem is the following:

bool startsWith(const String& s, bool caseSensitive) const;
bool startsWith(String& s, unsigned startOffset, bool caseSensitive = true) const;

A call like "consumedString.startsWith(string, caseSensitive)" matches both signatures because the compiler can do an internal casting of boolean to int, although for the commented call it picks the first method.

The warning message would be something like:

../../Source/WebCore/platform/text/SegmentedString.h:397:60: warning: ISO C++ says that these are ambiguous, even though the worst conversion for the first is better than the worst conversion for the second: [enabled by default]
         if (consumedString.startsWith(string, caseSensitive))
                                                            ^
In file included from ../../Source/WTF/wtf/text/AtomicString.h:26:0,
                 from ../../Source/WebCore/dom/EventListenerMap.h:39,
                 from ../../Source/WebCore/dom/EventTarget.h:35,
                 from ../../Source/WebCore/dom/Node.h:29,
                 from ../../Source/WebCore/dom/ContainerNode.h:28,
                 from ../../Source/WebCore/dom/DocumentFragment.h:27,
                 from ../../Source/WebCore/dom/DocumentFragment.cpp:24:
../../Source/WTF/wtf/text/WTFString.h:266:10: note: candidate 1: bool WTF::String::startsWith(const WTF::String&, bool) const
     bool startsWith(const String& s, bool caseSensitive) const
          ^
../../Source/WTF/wtf/text/WTFString.h:273:10: note: candidate 2: bool WTF::String::startsWith(const WTF::String&, unsigned int, bool)
     bool startsWith(const String& s, unsigned startOffset, bool caseSensitive = true)

Possible solutions:

   * Name the new methods differently so their signature doesn't collide with other existing methods.
   * Refactor the code to reduce ambiguity, although it could be a big refactoring that may affect many files. I think that if we are opting for this solution, it should be a different patch.

To avoid similar problems in "contains" I opted for the solution suggested on the last patch, which is adding an extra paratemer to the existing "contains" method. What I don't like much of this solution is that ends up with "contains" having a different signature than "endsWith" and "startsWith".

bool startsWith(String& s, unsigned startOffset, bool caseSensitive);
bool endsWith(String& s, unsigned endOffset, bool caseSensitive);
bool contains(const String& str, bool caseSensitive = true, unsigned startOffset = 0);

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