[Lldb-commits] [lldb] [lldb] In-progress — All ValueObjectSP instances are now valid (non-null) but have an error state (PR #74912)

[Pete Lawrence via lldb-commits]

PortalPete wrote:

> > ### Goal
> > Every `ValueObjectSP` will have an actual value and will never be equal to `nullptr`.
> 
> I would like to learn more about the goal. It seems like the existing code will result in widespread use of `optional<shared_ptr<ValueObject>>`. Is the plan to reduce these cases to a smaller amount? If not, then it's not clear to me how having the codebase predominantly use `optional<shared_ptr<ValueObject>>` is better than mostly using `shared_ptr<ValueObject>`. Is this refactoring a known (named) pattern? Are there other examples of where a `shared_ptr<T>` is replaced with `optional<shared_ptr<T>>`?

I believe this is a step towards using `class [[nodiscard]] Expected`, which @adrian-prantl can probably explain better than I can.

The reasoning behind the goal comes from all the places callers of these methods _only_ checking against `nullptr` when they should be checking its underlying error state (with `→GetError()`).

This change takes away the ability to use an `ValueObjectSP` instance as a Boolean, which lets the compiler tell us all the places where the code is doing just that, some of which are ok, and thee other places where it's not ok. Similarly, the hope is this restriction gets people in the habit of checking the value object's error state going forward.

https://github.com/llvm/llvm-project/pull/74912