Page MenuHomePhabricator

Add doxygen comments to the StackFrameList API (NFC)
ClosedPublic

Authored by vsk on Jul 31 2018, 10:55 AM.

Event Timeline

vsk created this revision.Jul 31 2018, 10:55 AM
labath accepted this revision.Aug 1 2018, 1:22 AM

I am not too familiar with this code, but the descriptions seem to make sense.

However, since you have kind of opened up the Optional discussion, I'll use this opportunity to give my take on it:

I've wanted to use Optional<IntType> in this way in the past, but the thing that has always stopped me is that this essentially doubles the amount of storage required for the value (you only need one bit for the IsValid field, but due to alignment constraints, this will usually consume the same amount of space as the underlying int type). This may not matter for the StackFrameList class, but it does matter for classes which have a lot of instances floating around. I suspect this is also why LLVM does not generally use this idiom (the last example where I ran into this is the VersionTuple class, which hand-rolls a packed optional by stealing a bit from the int type, and then converts this to Optional<unsigned> for the public accessors).

For this reason, I think it would be useful to have a specialized class (OptionalInt?), which has the same interface as Optional, but does not increase the storage size. It could be implemented the same way as we hand-roll the invalid values, only now the invalid values would be a part of the type. So, you could do something like:

using tid_t = OptionalInt<uint64_t, LLDB_INVALID_THREAD_ID>;

tid_t my_tid; // initialized to "invalid";
my_tid = get_thread_id();
if (my_tid) // no need to remember what is the correct "invalid" value here
  do_stuff(*my_tid);
else
  printf("no thread");

I am sure this is more than what you wanted to hear in this patch, but I wanted to make sure consider the tradeoffs before we start putting Optionals everywhere.

This revision is now accepted and ready to land.Aug 1 2018, 1:22 AM
vsk added a comment.Aug 1 2018, 9:15 AM

I am not too familiar with this code, but the descriptions seem to make sense.

However, since you have kind of opened up the Optional discussion, I'll use this opportunity to give my take on it:

I've wanted to use Optional<IntType> in this way in the past, but the thing that has always stopped me is that this essentially doubles the amount of storage required for the value (you only need one bit for the IsValid field, but due to alignment constraints, this will usually consume the same amount of space as the underlying int type). This may not matter for the StackFrameList class, but it does matter for classes which have a lot of instances floating around. I suspect this is also why LLVM does not generally use this idiom (the last example where I ran into this is the VersionTuple class, which hand-rolls a packed optional by stealing a bit from the int type, and then converts this to Optional<unsigned> for the public accessors).

For this reason, I think it would be useful to have a specialized class (OptionalInt?), which has the same interface as Optional, but does not increase the storage size. It could be implemented the same way as we hand-roll the invalid values, only now the invalid values would be a part of the type. So, you could do something like:

using tid_t = OptionalInt<uint64_t, LLDB_INVALID_THREAD_ID>;

tid_t my_tid; // initialized to "invalid";
my_tid = get_thread_id();
if (my_tid) // no need to remember what is the correct "invalid" value here
  do_stuff(*my_tid);
else
  printf("no thread");

I am sure this is more than what you wanted to hear in this patch, but I wanted to make sure consider the tradeoffs before we start putting Optionals everywhere.

No, I think that's a good point. I held off on introducing llvm::Optional here because of similar reservations. I think it'd make sense to introduce an OptionalInt facility, or possibly something more general that supports non-POD types & more exotic invalid values. Definitely something to revisit.

This revision was automatically updated to reflect the committed changes.