[webkit-reviews] review denied: [Bug 234571] Add WTF::UUID class which is natively a 128-bit integer : [Attachment 447879] EWS v2

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has denied Brady Eidson
<beidson at apple.com>'s request for review:
Bug 234571: Add WTF::UUID class which is natively a 128-bit integer
https://bugs.webkit.org/show_bug.cgi?id=234571

Attachment 447879: EWS v2

https://bugs.webkit.org/attachment.cgi?id=447879&action=review




--- Comment #3 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 447879
  --> https://bugs.webkit.org/attachment.cgi?id=447879
EWS v2

View in context: https://bugs.webkit.org/attachment.cgi?id=447879&action=review

> Source/WTF/wtf/UUID.cpp:47
> +    cryptographicallyRandomValues(reinterpret_cast<unsigned char*>(&m_data),
16);

static_assert(sizeof(m_data) == 16)

> Source/WTF/wtf/UUID.cpp:50
> +UUID::UUID(const Span<const uint8_t>& span)

I think this should take a Span<const uint8_t, 16> then the caller must ensure
the size is correct.
Also, Span is like StringView and doesn't need to be passed by reference.

> Source/WTF/wtf/UUID.cpp:52
> +    uint64_t high, low;

This seems unnecessary.  Can't we just memcpy into m_data directly?

> Source/WTF/wtf/UUID.cpp:59
> +Vector<uint8_t> UUID::toVector() const

I think this is always an unnecessary copy and allocation.  It could return a
Span<const uint8_t, 16>

> Source/WTF/wtf/UUID.cpp:74
> +    numbers[0] = UInt128High64(m_data);

This copy also seems unnecessary.

> Source/WTF/wtf/UUID.h:50
> +	   if (span.size() != 16)

That would remove this check and this constructor would always succeed.  Also,
I think create functions are a bit overkill here.