Clarify that some UUID routines are dedicated to UUID v4
Created attachment 449641 [details] Patch
Created attachment 449643 [details] Patch
Comment on attachment 449643 [details] 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?
> > 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. A few thoughts that push me towards using createVersion4UUIDString: - We are doing pseudo random number UUIDs, and spec is stating that they be called v4. By using Version4 in the routine name, we (try to) make it clear we are using pseudo random numbers. I am not sure what better name there is to convey that idea. - V4 cannot collide with empty and deleted values. Some code paths may never care, some code paths may not care right now but may care in the future. V4 is a safe default. - At least one code path requires V4. Probably most others do not care (maybe?). > > 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. There are examples with and without UL postfixes in the code bas . Given that, I assumed this was ok to use the shorter form (and the tests are indeed green). > > 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? The V4 code path will reject both of them later on. As of non v4, I am not sure, but the empty value is a predefined UUDI in the spec and is not harmful to parse, contrary to the deleted value we definitely prefer to create explicitly with the UUID constructor. > > Tools/TestWebKitAPI/Tests/WTF/UUID.cpp:66 > > + fprintf(stderr, "%s \n", createdString.utf8().data()); > > I think you don’t want to land this, right? Right.
<rdar://problem/88173227>
> I’m saying r=me, but I have one big bit of feedback. Let me know whether you'd prefer some further changes after my previous answer.
(In reply to youenn fablet from comment #6) > Let me know whether you'd prefer some further changes after my previous > answer. Thanks for asking. I think you made some great choices. I don’t think there’s anything major at this time that I see differently. Maybe I’ll have ideas later, but: great work and thanks for considering my suggestions and thoughts.
Created attachment 450618 [details] Patch for landing
Committed r288950 (246679@main): <https://commits.webkit.org/246679@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 450618 [details].