Page MenuHomePhabricator

Relax size constraints on UUID objects
AbandonedPublic

Authored by sas on Nov 27 2017, 6:18 PM.

Details

Summary

For some reason, the current implementation of UUID assumes all UUIDs
will always be either 16 or 20 bytes. I saw some 8-bytes long UUIDs on
some builds, and there doesn't seem to be any obvious reason why we
should enforce 16 or 20 byte-long UUIDs.

Ran check-lldb to ensure this doesn't break anything.

Event Timeline

sas created this revision.Nov 27 2017, 6:18 PM
zturner edited edge metadata.Nov 27 2017, 6:49 PM

If we're going to support arbitrarily sized UUIDs, maybe just make the class member a std::vector?

davide requested changes to this revision.Nov 27 2017, 9:40 PM

Stephane, can we please add unittests for UUID? [I don't see any of them in tree] It shouldn't be particularly hard.
If it is, I'd love to take this an example of why it's hard and discuss on how we can improve.
Thanks for your understanding :)

This revision now requires changes to proceed.Nov 27 2017, 9:40 PM
clayborg requested changes to this revision.Nov 28 2017, 10:12 AM

You can abandon https://reviews.llvm.org/D40537 and just do the fixes I suggested in that change here: init m_num_uuid_bytes to zero in constructor and in Clear() and change UUID::IsValid() to just return "m_num_uuid_bytes > 0". I agree we should relax the UUID length.

source/Utility/UUID.cpp
48
m_num_uuid_bytes = 0;
59–78

If we are going to relax the UUID size to anything between 0 and 20, then this function needs to print the correct number of digits. We shouldn't show more. This would allow us to set the UUID from the debug info CRC as a 4 byte UUID. Currently we display the UUID as a 20 byte version with only the first 4 bytes filled in.

102–103

Change to:

return m_num_uuid_bytes > 0;
sas abandoned this revision.Sep 6 2018, 10:20 AM