Bug 167087

Created attachment 298936 [details] Patch

I like the direction of this, but I am not sure I have nailed the ergonomics of the API quite yet, especially the FormattedNumber part, which feels clunky, but I wanted to put it up for discussion anyway.

I put the new adaptors in their own file because I was having a lot of trouble making it work in the main one. StringConcatenate.h is an odd puppy, with it's explicit #ifndef header guard checks. Ideally, this would all be in one file. The issue came down to dtoa.h causing some circular dependancies.

Comment on attachment 298936 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=298936&action=review Nice that you tested everything thoroughly, but where is the deployment at all the places using String::number today? > Source/WTF/wtf/text/AtomicString.h:366 > + typedef AtomicString ReturnType; > + typedef void AdditionalArgumentType; Newer code, so should start switching from typedef to using. > Source/WTF/wtf/text/IntegerToStringConversion.h:68 > +static void writeNumberToBufferImpl(UnsignedIntegerType number, CharacterType* destination) I always go out of my way to avoid "impl" in names like this. We should struggle a bit more. > Source/WTF/wtf/text/StringBuilder.h:374 > + typedef void ReturnType; > + typedef StringBuilder AdditionalArgumentType; Newer code, so should start switching from typedef to using. > Source/WTF/wtf/text/StringConcatenateNumbers.h:88 > + StringView(reinterpret_cast<const LChar*>(m_buffer), m_length).getCharactersWithUpconvert(destination); This just looks like a long way to write memcpy. > Source/WTF/wtf/text/StringConcatenateNumbers.h:93 > + StringView(reinterpret_cast<const LChar*>(m_buffer), m_length).getCharactersWithUpconvert(destination); Annoying to call a function that handles both 8-bit and 16-bit when we know our source is always 8-bit. > Source/WTF/wtf/text/StringConcatenateNumbers.h:96 > + String toString() const { return String(m_buffer, m_length); } String { } syntax instead of String()? > Source/WTF/wtf/text/StringConcatenateNumbers.h:110 > + StringTypeAdapter<double>(double number) > + { > + numberToString(number, m_buffer); > + m_length = strlen(m_buffer); > + } This should share a base class with StringTypeAdapter<float>. The constructor is the only function that is different. > Source/WTF/wtf/text/StringConcatenateNumbers.h:117 > + StringView(reinterpret_cast<const LChar*>(m_buffer), m_length).getCharactersWithUpconvert(destination); Ditto. > Source/WTF/wtf/text/StringConcatenateNumbers.h:122 > + StringView(reinterpret_cast<const LChar*>(m_buffer), m_length).getCharactersWithUpconvert(destination); Ditto. > Source/WTF/wtf/text/StringConcatenateNumbers.h:125 > + String toString() const { return String(m_buffer, m_length); } Ditto. > Source/WTF/wtf/text/StringConcatenateNumbers.h:173 > + m_numberFormatter.stringView().getCharactersWithUpconvert(destination); Ditto (same comment as in float and double). > Source/WTF/wtf/text/StringConcatenateNumbers.h:178 > + m_numberFormatter.stringView().getCharactersWithUpconvert(destination); Ditto. > Source/WTF/wtf/text/StringConcatenateNumbers.h:181 > + String toString() const { return String(m_numberFormatter.buffer(), m_numberFormatter.length()); } Ditto.

Committed r210790: <http://trac.webkit.org/changeset/210790>

Neat!