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

[Vedant Kumar via Phabricator via lldb-commits]

vsk added a comment.

In https://reviews.llvm.org/D50087#1183982, @labath wrote:

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


https://reviews.llvm.org/D50087