Bug 109947 – Add CString operators for comparison with const char*

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Chris Dumez]

CString currently has the following comparison operators:
bool operator==(const CString& a, const CString& b);
inline bool operator!=(const CString& a, const CString& b);

This means that if the caller does:
CString testString = "a";
if (testString == "a")
  // ...

It will construct a CString from the const char*, which involves a copy and a call to strlen(). I have seen such CString / const char* comparison done in WebKit.

We could add the following comparison operators to CString in order to make sure such comparison code is not expensive:
bool operator==(const CString& a, const char* b);
inline bool operator!=(const CString& a, const char* b);

[Chris Dumez]

Created attachment 188579 [details]
Patch

[Darin Adler]

Comment on attachment 188579 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=188579&action=review

Seems OK. Please fix the memcmp issue too if you are touching this code.

> Source/WTF/wtf/text/CString.cpp:113
> +    return !strncmp(a.data(), b.data(), a.length());

This code should use memcmp. We don’t want the code to scan for null characters, so strncmp is the wrong function.

[Benjamin Poulain]

Comment on attachment 188579 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=188579&action=review

This is great. Some comment bellow:

>> Source/WTF/wtf/text/CString.cpp:113
>> +    return !strncmp(a.data(), b.data(), a.length());
> 
> This code should use memcmp. We don’t want the code to scan for null characters, so strncmp is the wrong function.

Or StringImpl's equal(LChar, LChar, size_t).

> Source/WTF/wtf/text/CString.cpp:116
> +bool operator==(const CString& a, const char* b)

Would it be useful to have a version for literal?:
template<size_t bLength>
bool operator==(const CString& a, const char (&b)[bLength])

You could have compile assertions for bLength > 0, remove both branches, and go straight to memcmp/equal.

> Tools/TestWebKitAPI/Tests/WTF/CString.cpp:135
> +    CString c;
> +    const char* d = 0;
> +    ASSERT_TRUE(c == d);
> +    ASSERT_FALSE(c != d);

Another interesting boundary is empty string.

[Chris Dumez]

(In reply to comment #3)
> (From update of attachment 188579 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188579&action=review
> 
> This is great. Some comment bellow:
> 
> >> Source/WTF/wtf/text/CString.cpp:113
> >> +    return !strncmp(a.data(), b.data(), a.length());
> > 
> > This code should use memcmp. We don’t want the code to scan for null characters, so strncmp is the wrong function.
> 
> Or StringImpl's equal(LChar, LChar, size_t).
> 
> > Source/WTF/wtf/text/CString.cpp:116
> > +bool operator==(const CString& a, const char* b)
> 
> Would it be useful to have a version for literal?:
> template<size_t bLength>
> bool operator==(const CString& a, const char (&b)[bLength])
> 
> You could have compile assertions for bLength > 0, remove both branches, and go straight to memcmp/equal.

Would you like me to add it in the same patch?

> 
> > Tools/TestWebKitAPI/Tests/WTF/CString.cpp:135
> > +    CString c;
> > +    const char* d = 0;
> > +    ASSERT_TRUE(c == d);
> > +    ASSERT_FALSE(c != d);
> 
> Another interesting boundary is empty string.

Right, I forgot about this case. I'll improve the test case, thanks.

[Alexey Proskuryakov]

What code needs to compare CString to a literal?

I think of CString as a temporary object for platform interfaces, not something that should be used in places that have logic.

[Benjamin Poulain]

(In reply to comment #5)
> What code needs to compare CString to a literal?

EFL uses CString quite a bit, which is why I ask.

[Chris Dumez]

I don't know that the literal version would be useful yet. However, I have seen code in the soup backend comparing CString to const char* values returned by libsoup.

Can I keep the patch as it is then and simply improve the test case as suggested by Benjamin?

[Benjamin Poulain]

(In reply to comment #7)
> Can I keep the patch as it is then and simply improve the test case as suggested by Benjamin?

Yep + the comments of Darin. :)

[Chris Dumez]

(In reply to comment #8)
> (In reply to comment #7)
> > Can I keep the patch as it is then and simply improve the test case as suggested by Benjamin?
> 
> Yep + the comments of Darin. :)

Of course :)

[Chris Dumez]

Created attachment 188608 [details]
Patch for landing

Could someone please take a quick look before landing?

[Benjamin Poulain]

Comment on attachment 188608 [details]
Patch for landing

Thank you for updating the test.

[WebKit Review Bot]

Comment on attachment 188608 [details]
Patch for landing

Clearing flags on attachment: 188608

Committed r143049: <http://trac.webkit.org/changeset/143049>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Roger Fong]

This broke the Windows build.
Here's the output:
http://build-safari.apple.com/builders/Windows%20Release%20Archive/builds/19153/steps/build%20archive/logs/build%20log

Win EWS bots are really far behind :( ... might be why they bubble doesn't show up.

[Benjamin Poulain]

(In reply to comment #14)
> This broke the Windows build.
> Here's the output:
> http://build-safari.apple.com/builders/Windows%20Release%20Archive/builds/19153/steps/build%20archive/logs/build%20log
> 
> Win EWS bots are really far behind :( ... might be why they bubble doesn't show up.

It cannot find the symbol for the new operator==. Is it because of WTF_EXPORT_PRIVATE?
I forgot the difference between WTF_EXPORT_STRING_API, WTF_EXPORT_PRIVATE and WTF_EXPORT

[Chris Dumez]

(In reply to comment #15)
> (In reply to comment #14)
> > This broke the Windows build.
> > Here's the output:
> > http://build-safari.apple.com/builders/Windows%20Release%20Archive/builds/19153/steps/build%20archive/logs/build%20log
> > 
> > Win EWS bots are really far behind :( ... might be why they bubble doesn't show up.
> 
> It cannot find the symbol for the new operator==. Is it because of WTF_EXPORT_PRIVATE?
> I forgot the difference between WTF_EXPORT_STRING_API, WTF_EXPORT_PRIVATE and WTF_EXPORT

Sorry about that. According to http://trac.webkit.org/wiki/ExportingSymbols, we need to add the new symbol to JavaScriptCoreExports.def in addition to using WTF_EXPORT_PRIVATE.

However, it seems build.webkit.org and your link are both inaccessible at the moment :/

[Chris Dumez]

Windows build fix landed in http://trac.webkit.org/changeset/143099.