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);
Created attachment 188579 [details] Patch
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.
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.
(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.
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.
(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.
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?
(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. :)
(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 :)
Created attachment 188608 [details] Patch for landing Could someone please take a quick look before landing?
Comment on attachment 188608 [details] Patch for landing Thank you for updating the test.
Comment on attachment 188608 [details] Patch for landing Clearing flags on attachment: 188608 Committed r143049: <http://trac.webkit.org/changeset/143049>
All reviewed patches have been landed. Closing bug.
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.
(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
(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 :/
Windows build fix landed in http://trac.webkit.org/changeset/143099.