It would be nice to add a StringTypeAdapter for ASCIILiteral type so that code such as this one can be efficiently handled: String url = ASCIILiteral("file://") + path This was suggested by Benjamin Poulain in: https://bugs.webkit.org/show_bug.cgi?id=107811#c5
Created attachment 185489 [details] Patch
Comment on attachment 185489 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185489&action=review > Source/WTF/wtf/text/StringConcatenate.h:276 > + , m_length(strlen(buffer)) shouldn't it be checked that buffer!=0? or was it already checked before? > Source/WTF/wtf/text/StringConcatenate.h:280 > + size_t length() { return m_length; } is there any reason why this is not const method? (I know that you have to be aligned with StringTypeAdapter declaration but I'm just curious :) ) > Source/WTF/wtf/text/StringConcatenate.h:282 > + bool is8Bit() { return true; } ditto.
Comment on attachment 185489 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185489&action=review >> Source/WTF/wtf/text/StringConcatenate.h:276 >> + , m_length(strlen(buffer)) > > shouldn't it be checked that buffer!=0? or was it already checked before? Other StringAdapters do not have NULL-checks either. I guess getting NULL may be possible if someone writes "ASCIILiteral(0) + str". However, this would be wrong usage of ASCIILiteral IMO. >> Source/WTF/wtf/text/StringConcatenate.h:280 >> + size_t length() { return m_length; } > > is there any reason why this is not const method? (I know that you have to be aligned with StringTypeAdapter declaration but I'm just curious :) ) I don't mind making those getter const but I chose to be consistent with other StringTypeAdapters.
Comment on attachment 185489 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185489&action=review > Source/WTF/wtf/text/StringConcatenate.h:286 > + memcpy(destination, m_buffer, static_cast<size_t>(m_length) * sizeof(LChar)); Silly to write * sizeof(LChar) since that’s defined to be 1. > Source/WTF/wtf/text/StringOperators.h:136 > +inline StringAppend<ASCIILiteral, String> operator+(ASCIILiteral string1, const String& string2) Shouldn’t this be const ASCIILiteral&? > Source/WTF/wtf/text/StringOperators.h:141 > +inline StringAppend<ASCIILiteral, AtomicString> operator+(ASCIILiteral string1, const AtomicString& string2) Shouldn’t this be const ASCIILiteral&? > Source/WTF/wtf/text/StringOperators.h:147 > +template<typename U, typename V> > +StringAppend<ASCIILiteral, StringAppend<U, V> > operator+(ASCIILiteral string1, const StringAppend<U, V>& string2) This should definitely be marked inline. If the others like this in the file aren’t, that’s a bug. Shouldn’t this be const ASCIILiteral&?
Comment on attachment 185489 [details] Patch Attachment 185489 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16218582 New failing tests: svg/as-image/img-relative-height.html
Comment on attachment 185489 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185489&action=review >> Source/WTF/wtf/text/StringConcatenate.h:286 >> + memcpy(destination, m_buffer, static_cast<size_t>(m_length) * sizeof(LChar)); > > Silly to write * sizeof(LChar) since that’s defined to be 1. Ok. This is done elsewhere in the file so I used the same but I can remove it. >> Source/WTF/wtf/text/StringOperators.h:136 >> +inline StringAppend<ASCIILiteral, String> operator+(ASCIILiteral string1, const String& string2) > > Shouldn’t this be const ASCIILiteral&? Well, ASCIILiteral is usually passed by value elsewhere. See for example the WTF::String(ASCIILiteral) constructor. I was assuming this was made on purpose because ASCIILiteral only contains 1 member that is a pointer. Passing by value in this case is likely faster in this case (http://www.macieira.org/blog/2012/02/the-value-of-passing-by-value/). If it was not made on purpose, then the WTF::String constructor should probably be updated? >> Source/WTF/wtf/text/StringOperators.h:147 >> +StringAppend<ASCIILiteral, StringAppend<U, V> > operator+(ASCIILiteral string1, const StringAppend<U, V>& string2) > > This should definitely be marked inline. If the others like this in the file aren’t, that’s a bug. > > Shouldn’t this be const ASCIILiteral&? Ok, I'll check.
Created attachment 185520 [details] Patch Take Darin's comments into consideration: - Inline remaining operator+ functions - Remove '* sizeof(LChar)' in memcpy Note that I'm still passing the ASCIILiteral object by value based on my earlier comment. Benjamin can hopefully confirm that he is did this on purpose.
> Other StringAdapters do not have NULL-checks either. I guess getting NULL may be possible if someone writes "ASCIILiteral(0) + str". However, this would be wrong usage of ASCIILiteral IMO. Yep, that is indeed by design. Some future work will enforce that (when ASCIILiteral will know its size). > Well, ASCIILiteral is usually passed by value elsewhere. See for example the WTF::String(ASCIILiteral) constructor. I was assuming this was made on purpose because ASCIILiteral only contains 1 member that is a pointer. Passing by value in this case is likely faster in this case (http://www.macieira.org/blog/2012/02/the-value-of-passing-by-value/). If it was not made on purpose, then the WTF::String constructor should probably be updated? WTF::String take an ASCIILiteral string by design (for the reasons you mention). That case is a bit different because that function is not inline. Here I think it would read better to have it const&.
Comment on attachment 185520 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185520&action=review Thank you for fixing this. Can you please also extend the tests: Tools/TestWebKitAPI/Tests/WTF/StringOperators.cpp? > Source/WTF/wtf/text/StringConcatenate.h:274 > + StringTypeAdapter<ASCIILiteral>(const char* buffer) I would not rely on the implicit conversion for the constructor. I think an explicit conversion would read better. > Source/WTF/wtf/text/StringConcatenate.h:292 > + for (unsigned i = 0; i < m_length; ++i) > + destination[i] = m_buffer[i]; We have copyChars() in StringImpl to do exactly this. :)
Created attachment 185593 [details] Patch Take Benjamin's feedback into consideration.
Comment on attachment 185593 [details] Patch Nice test.
Comment on attachment 185593 [details] Patch Clearing flags on attachment: 185593 Committed r141342: <http://trac.webkit.org/changeset/141342>
All reviewed patches have been landed. Closing bug.