Re: [Lldb-commits] [PATCH] D40537: Simplify UUID::IsValid()

For that you still need something like what we have, but it would be nice if the internal code that knew more about the contents could use that extra information for better clarity and safety. I’d honestly probably make the interfaces deal in ArrayRef and assert in the API boundaries that that the

Re: [Lldb-commits] [PATCH] D40537: Simplify UUID::IsValid()

How would the ObjectFile API's that take or return UUID's work in this case? Jim > On Nov 28, 2017, at 11:44 AM, Zachary Turner via lldb-commits > wrote: > > Also worth pointing out that when you write things this way, this UUID > class can be part of a larger structure that matches the reco

Re: [Lldb-commits] [PATCH] D40537: Simplify UUID::IsValid()

Also worth pointing out that when you write things this way, this UUID class can be part of a larger structure that matches the record layout of a header or section in a binary file, and then you can just memcpy over the class and your'e good to go. For example you could have ``` struct MachOHead

Re: [Lldb-commits] [PATCH] D40537: Simplify UUID::IsValid()

Eh, that actually just makes me think the compiler *can* check it. For example, right now you can have mach-o files with 20 byte UUIDs. But just in the code, not in practice. You could have a bug in your code that accidentally wrote the wrong number of bytes from a dynamic buffer. You could enf

Re: [Lldb-commits] [PATCH] D40537: Simplify UUID::IsValid()

> On Nov 28, 2017, at 10:24 AM, Zachary Turner wrote: > > > > On Tue, Nov 28, 2017 at 10:18 AM Greg Clayton > wrote: >> On Nov 27, 2017, at 10:11 PM, Zachary Turner > > wrote: >> >> As an aside, I don't really like this class. For exampl

Re: [Lldb-commits] [PATCH] D40537: Simplify UUID::IsValid()

On Tue, Nov 28, 2017 at 10:18 AM Stephane Sezer via Phabricator < revi...@reviews.llvm.org> wrote: > > > The other alternative seems a bit less explicit to me but I don't mind it > too much. What's the issue with using `std::any_of` exactly? > > In the sense of correctness, nothing is wrong with u

Re: [Lldb-commits] [PATCH] D40537: Simplify UUID::IsValid()

On Tue, Nov 28, 2017 at 10:24 AM Zachary Turner wrote: > On Tue, Nov 28, 2017 at 10:18 AM Greg Clayton wrote: > >> On Nov 27, 2017, at 10:11 PM, Zachary Turner wrote: >> >> As an aside, I don't really like this class. For example, You can >> currently assign a UUID[16] to a UUID[20]. That doe

Re: [Lldb-commits] [PATCH] D40537: Simplify UUID::IsValid()

On Tue, Nov 28, 2017 at 10:18 AM Greg Clayton wrote: > On Nov 27, 2017, at 10:11 PM, Zachary Turner wrote: > > As an aside, I don't really like this class. For example, You can > currently assign a UUID[16] to a UUID[20]. That doesn't make a lot of > sense to me. > > > What about an invalid UU

[Lldb-commits] [PATCH] D40537: Simplify UUID::IsValid()

clayborg added a comment. No one is relying on the 16 bytes of zeroes that I know of. UUID::IsValid() is the test that everyone uses to tell if the UUID is valid or not. I still prefer to just set the length to zero as this does allow us to have a UUID of all zeroes if needed. https://reviews

Re: [Lldb-commits] [PATCH] D40537: Simplify UUID::IsValid()

> On Nov 27, 2017, at 10:11 PM, Zachary Turner wrote: > > As an aside, I don't really like this class. For example, You can currently > assign a UUID[16] to a UUID[20]. That doesn't make a lot of sense to me. What about an invalid UUID[0] being assigned with a valid UUID[16] or UUID[20]? Wh

[Lldb-commits] [PATCH] D40537: Simplify UUID::IsValid()

sas added a comment. In https://reviews.llvm.org/D40537#937880, @zturner wrote: > In https://reviews.llvm.org/D40537#937866, @sas wrote: > > > In https://reviews.llvm.org/D40537#937196, @zturner wrote: > > > > > You could use llvm's range adapters to make this slightly better. > > > > > > auto

[Lldb-commits] [PATCH] D40537: Simplify UUID::IsValid()

zturner added a comment. In https://reviews.llvm.org/D40537#937866, @sas wrote: > In https://reviews.llvm.org/D40537#937196, @zturner wrote: > > > You could use llvm's range adapters to make this slightly better. > > > > auto Bytes = makeArrayRef(m_uuid, m_num_uuid_bytes); > > return llvm::fi

[Lldb-commits] [PATCH] D40537: Simplify UUID::IsValid()

sas added a comment. In https://reviews.llvm.org/D40537#937862, @clayborg wrote: > A better solution would be to initialize UUID::m_num_uuid_bytes with zero and > only set it to a valid value if we set bytes into it. Then UUID::IsValid() > becomes easy: > > bool UUID::IsValid() const { return

[Lldb-commits] [PATCH] D40537: Simplify UUID::IsValid()

sas added a comment. In https://reviews.llvm.org/D40537#937196, @zturner wrote: > You could use llvm's range adapters to make this slightly better. > > auto Bytes = makeArrayRef(m_uuid, m_num_uuid_bytes); > return llvm::find(Bytes, 0) != Bytes.end(); > > > Another option would just be `ret

[Lldb-commits] [PATCH] D40537: Simplify UUID::IsValid()

clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. A better solution would be to initialize UUID::m_num_uuid_bytes with zero and only set it to a valid value if we set bytes into it. Then UUID::IsValid() becomes easy: bool UUI

Re: [Lldb-commits] [PATCH] D40537: Simplify UUID::IsValid()

As an aside, I don't really like this class. For example, You can currently assign a UUID[16] to a UUID[20]. That doesn't make a lot of sense to me. As a future cleanup, I think this class should probably be a template such as UUID, and then internally it can store a std::array. And we can stat

[Lldb-commits] [PATCH] D40537: Simplify UUID::IsValid()

davide added a comment. Yes, what Zachary said. Thanks for the cleanup. There's a decent amount of code in lldb that can be written in a very compact way but it's manually unrolled. I think in this case the code produced should be equivalent (and if not, I'd be curious to see the codegen). More

[Lldb-commits] [PATCH] D40537: Simplify UUID::IsValid()

zturner added a comment. You could use llvm's range adapters to make this slightly better. auto Bytes = makeArrayRef(m_uuid, m_num_uuid_bytes); return llvm::find(Bytes, 0) != Bytes.end(); Another option would just be `return *this != UUID(m_num_uuid_bytes);` https://reviews.llvm.org/D40537

[Lldb-commits] [PATCH] D40537: Simplify UUID::IsValid()

sas created this revision. This change also prevents looking further than the size of the current UUID. https://reviews.llvm.org/D40537 Files: source/Utility/UUID.cpp Index: source/Utility/UUID.cpp === --- source/Utility/UUID.c