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

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added a comment.

> 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?



================
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];
 
----------------
switch to llvm::SmallVector<uint8_t, 20> as suggested by Greg? 


================
Comment at: include/lldb/Utility/UUID.h:42
 
-  const UUID &operator=(const UUID &rhs);
+  UUID &operator=(const UUID &rhs) = default;
 
----------------
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)


================
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; }
----------------
is this really needed? I'd prefer the truly explicit (pun intended) IsValid()


================
Comment at: include/lldb/Utility/UUID.h:95
 
 bool operator==(const UUID &lhs, const UUID &rhs);
 bool operator!=(const UUID &lhs, const UUID &rhs);
----------------
these can be simplified if we use llvm::SmallVector (or std::vector) 


https://reviews.llvm.org/D48479