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

[Leonard Mosescu via lldb-commits]

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


 One solution might be to encapsulate the MachO convention in the MachO
code: check in there (maybe through a helper function) if the UUID is
"000...0" and map it to the empty UUID in that case. The UUID interface
would not have to know/care about this convention. Would this work?

On Fri, Jun 22, 2018 at 12:02 PM, Pavel Labath via Phabricator <
reviews at reviews.llvm.org> wrote:

> 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
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20180622/08432acf/attachment.html>