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