Page MenuHomePhabricator

Simplify UUID::IsValid()
AbandonedPublic

Authored by sas on Nov 27 2017, 6:14 PM.

Details

Summary

This change also prevents looking further than the size of the current
UUID.

Event Timeline

sas created this revision.Nov 27 2017, 6:14 PM
zturner edited edge metadata.Nov 27 2017, 6:46 PM

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);

davide edited edge metadata.Nov 27 2017, 9:38 PM

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 generally, whenever a function is not in a hot loop, we might consider preferring readability over performances anyway.

clayborg requested changes to this revision.Nov 28 2017, 10:06 AM

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 m_num_uuid_bytes > 0; }

This would allows us to actually have a UUID value that is valid and all zeroes. A few comments would need to be fixed as it currently assumes length is 16 or 20.

This revision now requires changes to proceed.Nov 28 2017, 10:06 AM
sas added a comment.Nov 28 2017, 10:10 AM

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);

We want at least one non-zero element, we don't want a zero-element, so the proposed code wouldn't work. I'm not sure if there's an llvm utility that allows doing that directly without having to pass a lambda.

sas added a comment.Nov 28 2017, 10:11 AM

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 m_num_uuid_bytes > 0; }

This would allows us to actually have a UUID value that is valid and all zeroes. A few comments would need to be fixed as it currently assumes length is 16 or 20.

Yes but the current default constructor of the UUID class creates a 16-bytes all-zeroes UUID. I'm not sure I want to be changing the default behavior that the rest of lldb might be depending on currently.

zturner added a comment.EditedNov 28 2017, 10:13 AM
In D40537#937866, @sas 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 return *this != UUID(m_num_uuid_bytes);

We want at least one non-zero element, we don't want a zero-element, so the proposed code wouldn't work. I'm not sure if there's an llvm utility that allows doing that directly without having to pass a lambda.

Wouldn't the other alternative work, where you just use operator== against a default constructed instance?

sas added a comment.Nov 28 2017, 10:18 AM
In D40537#937866, @sas 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 return *this != UUID(m_num_uuid_bytes);

We want at least one non-zero element, we don't want a zero-element, so the proposed code wouldn't work. I'm not sure if there's an llvm utility that allows doing that directly without having to pass a lambda.

Wouldn't the other alternative work, where you just use operator== against a default constructed instance?

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?

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.

sas abandoned this revision.Sep 6 2018, 10:19 AM