JDevlieghere requested changes to this revision.
JDevlieghere added a comment.
This revision now requires changes to proceed.
> I understand one could pass some better abstracted error-reporting interface
> instead of ValueObject itself but that would require some more code
> (interface definition, inheritance etc.).
How much work would it be to return an `llvm::Expected<bool>` from `Get_Impl`?
I'd really prefer that over the current approach.
================
Comment at: lldb/include/lldb/Core/ValueObject.h:459
+ void SetError(Status &&error) { m_error = std::move(error); }
+
----------------
This allows overwriting the error without checking if it's already set. Maybe
it's better to use `GetError`, check that it's not set, and then either set it
or append to it?
================
Comment at: lldb/include/lldb/DataFormatters/FormattersContainer.h:296
- bool Get_Impl(ConstString key, MapValueType &value,
+ bool Get_Impl(ConstString key, MapValueType &value, ValueObject *valobj,
lldb::RegularExpressionSP *dummy) {
----------------
If returning an expected isn't feasible, we should pass at least a `Status*`
instead of setting the error in the `ValueObject` directly.
================
Comment at: lldb/source/DataFormatters/TypeCategory.cpp:303
+ if (GetTypeFormatsContainer()->Get(type_name, format_sp,
+ nullptr /*valobj*/)) {
if (matching_category)
----------------
It's really unfortunate that so many call sites now require a null pointer. Can
we make it a default argument?
Repository:
rLLDB LLDB
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66654/new/
https://reviews.llvm.org/D66654
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
