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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
