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
_______________________________________________
lldb-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
