Summary: | Add CString operators for comparison with const char* | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||
Component: | Web Template Framework | Assignee: | Chris Dumez <cdumez> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | andersca, ap, benjamin, cmarcelo, laszlo.gombos, ojan.autocc, roger_fong, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Chris Dumez
2013-02-15 08:23:23 PST
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. |