[webkit-reviews] review granted: [Bug 235430] Clarify that some UUID routines are dedicated to UUID v4 : [Attachment 449643] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted youenn fablet <youennf at gmail.com>'s
request for review:
Bug 235430: Clarify that some UUID routines are dedicated to UUID v4
https://bugs.webkit.org/show_bug.cgi?id=235430

Attachment 449643: Patch

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 449643
  --> https://bugs.webkit.org/attachment.cgi?id=449643
Patch

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

I’m saying r=me, but I have one big bit of feedback.

> Source/WTF/ChangeLog:3
> +	   Clarify that some UUID routines are dedicated to UUID v4

Really glad you are following through on this!

> Source/WTF/ChangeLog:10
> +	   Rename UUID::create to UUID::createVersion4.
> +	   Add a UUID::parseVersion4 that validates the parsed UUID is a v4.
> +	   Rename createCanonicalUUIDString to createVersion4UUIDString for
consistency.

I’m not sure I like this. For both UUID::create and createCanonicalUUIDString,
seems the most common case is to want to create a UUID and not care which type.
Whichever is the fastest good UUID, presumably? Seems like there would be a
function to create a new UUID for general purpose uses, and in cases where I
don’t care which type of UUID is created, I shouldn’t have to say in the
function name which kind I want. But then there would be other cases where I
specifically need a v4 UUID, then I would want to call a function with the
more-specific name to help us keep track of the fact that this place requires a
v4 UUID.

We’ve done that for parse, where there is one function that can parse any UUID
(although it comes with the bonus feature of rejecting the pattern we reserved
for our deleted value), and a different one for cases where we need to check
that it’s v4 specifically.

I think we should keep that for create, and that means UUID::create would still
exist and would not specify v4 even though that is its current implementation.
Not sure what we would name createCanonicalUUIDString, though, since I have no
idea what the word "Canonical" is doing in there, and nothing about that names
seems to shout "new UUID" to me. It sounds like what is new is the string, and
maybe the UUID isn’t a newly created one.

> Source/WTF/wtf/UUID.cpp:55
> +    auto high = static_cast<uint64_t>((m_data >> 64) & 0xffffffffffff0fff) |
0x4000;
> +    auto low = static_cast<uint64_t>(m_data & 0x3fffffffffffffff) |
0x8000000000000000;

Do those long hex constants correctly give us 64-bit? Typically I assume that
constants will be int unless we add suffixes with letters like "u" for unsigned
and "l" for long. And that would make this code not work. So I assume that’s
not right? I guess the tests are working, so maybe I just don’t know the latest
rules for this.

> Source/WTF/wtf/UUID.cpp:121
> -    if (result == emptyValue || result == deletedValue)
> +    if (result == deletedValue)

What’s the rationale here? We want to successfully parse the empty value? We
want to reject the deleted value? Are there any callers that want to reject the
empty value?

> Tools/TestWebKitAPI/Tests/WTF/UUID.cpp:66
> +	   fprintf(stderr, "%s \n", createdString.utf8().data());

I think you don’t want to land this, right?