Bug 108338 – Add a StringTypeAdapter for ASCIILiteral

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

[Chris Dumez]

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

[Chris Dumez]

Created attachment 185489 [details]
Patch

[Mikhail Pozdnyakov]

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.

[Chris Dumez]

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.

[Darin Adler]

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&?

[Build Bot]

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

[Chris Dumez]

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.

[Chris Dumez]

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.

[Benjamin Poulain]

> 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&.

[Benjamin Poulain]

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. :)

[Chris Dumez]

Created attachment 185593 [details]
Patch

Take Benjamin's feedback into consideration.

[Benjamin Poulain]

Comment on attachment 185593 [details]
Patch

Nice test.

[WebKit Review Bot]

Comment on attachment 185593 [details]
Patch

Clearing flags on attachment: 185593

Committed r141342: <http://trac.webkit.org/changeset/141342>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.