[Lldb-commits] [PATCH] D48479: Represent invalid UUIDs as UUIDs with length zero

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In https://reviews.llvm.org/D48479#1140927, @lemo wrote:

> > The slight complication here is that
> >  some clients (MachO) actually use the all-zero notation to mean "no UUID
> >  has been set". To keep this use case working, I have introduced an
> >  additional argument to the UUID constructor, which specifies whether an
> >  all-zero vector should be considered a valid UUID. For the usages where
> >  the UUID data comes from a MachO file, I set this argument to false.
>
> What is the distinction between "no UUID has been set" and "invalid UUID"? I find this subtlety confusing, can we simplify it to just valid/not-valid UUIDs?


That is what I am trying to do (although not completely successfully, it seems ;) ). Once you have a UUID object around there should be no distinction. You either have a valid uuid (for now, consisting of 16 or 20 bytes), or you don't.

However, during parsing you need to know the meaning of a "0000...0" UUID. In a MachO file (at least based on the comments in the code) this value is used to denote the fact that the object file has no UUID. For elf, a "000..0" build-id is a perfectly valid identifier (and the lack of a build-id is denoted by the absence of the build-id section). The extra constructor argument is my way of trying to support both scenarios. The other possibility I see is to have a some kind of a factory function for one of the options (or both). I don't really have a preference between the two.



================
Comment at: include/lldb/Utility/UUID.h:31
   // Most UUIDs are 16 bytes, but some Linux build-ids (SHA1) are 20.
   typedef uint8_t ValueType[20];
 
----------------
lemo wrote:
> switch to llvm::SmallVector<uint8_t, 20> as suggested by Greg? 
I'm deliberately keeping that for a separate patch. Here, I just want to prepare the ground by defining the "invalid" UUID more clearly. The part with the arbitrary UUID length will come after that.


================
Comment at: include/lldb/Utility/UUID.h:42
 
-  const UUID &operator=(const UUID &rhs);
+  UUID &operator=(const UUID &rhs) = default;
 
----------------
lemo wrote:
> If we define this we should define at least the copy constructor as well (rule of 3/5). In this case the cleanest solution may be to allow the implicit definitions (which would also allow the implicit move operations - defining assignment operator as defaulted would inhibit the implicit move SMFs)
Good point. I've deleted the copy constructor altogether.


================
Comment at: include/lldb/Utility/UUID.h:52
 
-  bool IsValid() const;
+  explicit operator bool() const { return IsValid(); }
+  bool IsValid() const { return m_num_uuid_bytes > 0; }
----------------
lemo wrote:
> is this really needed? I'd prefer the truly explicit (pun intended) IsValid()
My main reason for an `operator bool` is that it allows the if-declaration syntax (`if (UUID u = getUUID())  doSomethingUsefulWith(u);` The most llvm-y solution would be not have neither of these methods and use Optional<UUID> when you don't know if you have one, but as far as operator bool vs. isValid goes, both styles are used in llvm (and lldb).


https://reviews.llvm.org/D48479