[webkit-reviews] review granted: [Bug 56099] Introduce WTF HexNumber.h : [Attachment 86262] Patch v8

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 56099: Introduce WTF HexNumber.h
https://bugs.webkit.org/show_bug.cgi?id=56099

Attachment 86262: Patch v8
https://bugs.webkit.org/attachment.cgi?id=86262&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=86262&action=review

> Source/JavaScriptCore/wtf/HexNumber.h:76
> +    Vector<UChar, 8> result;
> +    do {
> +	   result.prepend(hexDigits[number % 16]);
> +	   number >>= 4;
> +    } while (number > 0);

We are guaranteed that we never need more than 8 digits for this. So we should
write this with an array, not a vector; it would be significantly more
efficient since we could put the data at the end of the array and would not
need to move everything down as Vector does when you prepend.

> Source/JavaScriptCore/wtf/HexNumber.h:81
> +// Same as appendUsingAsHex, but using exactly 'desiredDigits' for the
conversion.

Typo here: Using instead of Unsigned.

> Source/JavaScriptCore/wtf/HexNumber.h:83
> +inline void appendUnsignedAsHexFixedSize(unsigned number, T& destination,
unsigned desiredDigits, HexConversionMode mode = Uppercase)

I would say AsFixedSizeHex rather than AsHexFixedSize.

> Source/JavaScriptCore/wtf/HexNumber.h:92
> +    const char* hexDigits = Internal::hexDigitsForMode(mode);
> +    Vector<UChar, 8> result;
> +    do {
> +	   result.prepend(hexDigits[number % 16]);
> +	   number >>= 4;
> +    } while (result.size() < desiredDigits);

As long as desiredDigits is only allowed to be in the range 1-8, we could do
the same as I suggest above, using an array instead of a vector and starting at
the end of the array.

> Source/WebCore/platform/UUID.cpp:110
> +    appendUnsignedAsHexFixedSize((randomData[2] >> 30) | 0x8, builder, 1,
Lowercase);

This could use the non-fixed-size version. Fixed size seems OK, though.