<div dir="ltr"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span style="font-size:12.8px;text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">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.</span></blockquote><div><br></div><div> 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?</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jun 22, 2018 at 12:02 PM, Pavel Labath via Phabricator <span dir="ltr"><<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">labath added a comment.<br>
<span class=""><br>
In <a href="https://reviews.llvm.org/D48479#1140927" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D48479#1140927</a>, @lemo wrote:<br>
<br>
> > The slight complication here is that<br>
> >  some clients (MachO) actually use the all-zero notation to mean "no UUID<br>
> >  has been set". To keep this use case working, I have introduced an<br>
> >  additional argument to the UUID constructor, which specifies whether an<br>
> >  all-zero vector should be considered a valid UUID. For the usages where<br>
> >  the UUID data comes from a MachO file, I set this argument to false.<br>
><br>
</span><span class="">> 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?<br>
<br>
<br>
</span>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.<br>
<br>
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.<br>
<span class=""><br>
<br>
<br>
================<br>
Comment at: include/lldb/Utility/UUID.h:31<br>
   // Most UUIDs are 16 bytes, but some Linux build-ids (SHA1) are 20.<br>
   typedef uint8_t ValueType[20];<br>
<br>
----------------<br>
</span><span class="">lemo wrote:<br>
> switch to llvm::SmallVector<uint8_t, 20> as suggested by Greg? <br>
</span>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.<br>
<span class=""><br>
<br>
================<br>
Comment at: include/lldb/Utility/UUID.h:42<br>
<br>
-  const UUID &operator=(const UUID &rhs);<br>
+  UUID &operator=(const UUID &rhs) = default;<br>
<br>
----------------<br>
</span><span class="">lemo wrote:<br>
> 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)<br>
</span>Good point. I've deleted the copy constructor altogether.<br>
<span class=""><br>
<br>
================<br>
Comment at: include/lldb/Utility/UUID.h:52<br>
<br>
-  bool IsValid() const;<br>
+  explicit operator bool() const { return IsValid(); }<br>
+  bool IsValid() const { return m_num_uuid_bytes > 0; }<br>
----------------<br>
</span><span class="">lemo wrote:<br>
> is this really needed? I'd prefer the truly explicit (pun intended) IsValid()<br>
</span>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).<br>
<br>
<br>
<a href="https://reviews.llvm.org/D48479" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D48479</a><br>
<br>
<br>
<br>
</blockquote></div><br></div>