[Lldb-commits] [PATCH] D50087: Add doxygen comments to the StackFrameList API (NFC)

Pavel Labath via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Wed Aug 1 01:22:03 PDT 2018

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

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
    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.


More information about the lldb-commits mailing list