Bug 234571

Created attachment 447857 [details] EWS v1

Created attachment 447879 [details] EWS v2

Comment on attachment 447879 [details] 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.

Created attachment 447883 [details] v3

Comment on attachment 447883 [details] v3 View in context: https://bugs.webkit.org/attachment.cgi?id=447883&action=review > Source/WTF/wtf/UUID.cpp:57 > + return StringHasher::hashMemory(numbers, 16); Can't we just call this on &m_data?

Created attachment 447889 [details] PFL v1

Created attachment 447900 [details] PFL v2

Committed r287412 (245547@main): <https://commits.webkit.org/245547@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 447900 [details].

<rdar://problem/86864395>

Comment on attachment 447900 [details] PFL v2 View in context: https://bugs.webkit.org/attachment.cgi?id=447900&action=review > Source/WTF/wtf/UUID.cpp:48 > + cryptographicallyRandomValues(reinterpret_cast<unsigned char*>(&m_data), 16); Seems misleading to call these UUIDs if they don’t have a conforming version number (which would be 4 for random UUIDs). Should this use something like the uuid_generate_random (<https://github.com/apple-oss-distributions/Libc/blob/Libc-1439.141.1/uuid/uuidsrc/gen_uuid.c#L238>) instead?

Comment on attachment 447900 [details] PFL v2 View in context: https://bugs.webkit.org/attachment.cgi?id=447900&action=review >> Source/WTF/wtf/UUID.cpp:48 >> + cryptographicallyRandomValues(reinterpret_cast<unsigned char*>(&m_data), 16); > > Seems misleading to call these UUIDs if they don’t have a conforming version number (which would be 4 for random UUIDs). Should this use something like the uuid_generate_random (<https://github.com/apple-oss-distributions/Libc/blob/Libc-1439.141.1/uuid/uuidsrc/gen_uuid.c#L238>) instead? I came here to write something similar; I don’t think is accurate or safe to generate 128 random bits and call that a UUID. > Source/WTF/wtf/UUID.cpp:53 > + return StringHasher::hashMemory(reinterpret_cast<const unsigned char*>(&m_data), 16); I suspect we can has the 128 bits more efficiently. I suggest we change this to call intHash() and write an integer hash for 128 bits analogous to the ones we have for 8, 16, 32, and 64 bit integers. Might be tricky because of UInt128. > Source/WTF/wtf/UUID.h:41 > +WTF_MAKE_FAST_ALLOCATED; Why are we including this? Are we allocating UUID objects on the heap? I suggest omitting this. > Source/WTF/wtf/UUID.h:53 > + explicit UUID(UInt128Impl&& data) Since UInt128Impl is a structure containing only scalars, we should just take a UInt128Impl, not a UInt128Impl&&. There’s no ownership transfer here. This should use UInt128, not UInt128Impl. > Source/WTF/wtf/UUID.h:58 > + UUID(const UUID&) = default; If we omit this, it should be correctly generated. > Source/WTF/wtf/UUID.h:60 > + Span<const uint8_t, 16> toSpan() const This can just be a conversion operator to Span. We can also have the toSpan function if you like, for cases where conversion would otherwise be ambiguous. But if we had a conversion operator I am pretty sure we could just write: Span span = identifier; Or if that doesn’t compile, define a UUIDBytesSpan type to get the length right: UUIDBytesSpan span = identifier; > Source/WTF/wtf/UUID.h:65 > + UUID& operator=(const UUID&) = default; If we omit this, it should be correctly generated. > Source/WTF/wtf/UUID.h:87 > + UInt128Impl m_data; This should use UInt128, not UInt128Impl.

Comment on attachment 447900 [details] PFL v2 View in context: https://bugs.webkit.org/attachment.cgi?id=447900&action=review Nice >>> Source/WTF/wtf/UUID.cpp:48 >>> + cryptographicallyRandomValues(reinterpret_cast<unsigned char*>(&m_data), 16); >> >> Seems misleading to call these UUIDs if they don’t have a conforming version number (which would be 4 for random UUIDs). Should this use something like the uuid_generate_random (<https://github.com/apple-oss-distributions/Libc/blob/Libc-1439.141.1/uuid/uuidsrc/gen_uuid.c#L238>) instead? > > I came here to write something similar; I don’t think is accurate or safe to generate 128 random bits and call that a UUID. Can we ensure that m_data is not 0 or 1? According to newly added HashTraits<UUID>, 0 is used for empty and 1 is used for deleted value. And I think this random value can be 0 or 1 currently. > Source/WTF/wtf/UUID.h:106 > + encoder << UInt128High64(m_data) << UInt128Low64(m_data); Use the following, which is more efficient in architectures which support __uint128_t. encoder << static_cast<uint64_t>(m_data >> 64) << static_cast<uint64_t>(m_data); > Source/WTF/wtf/UUID.h:124 > + return { UUID { > + MakeUInt128(*high, *low), > + } }; Let's just do this instead of using MakeUInt128. return UUID { (static_cast<UInt128>(*high) << 64) | *low };