[Lldb-commits] [PATCH] D66654: Prevent D66398 `TestDataFormatterStdList`-like regressions in the future

[Jonas Devlieghere via Phabricator via lldb-commits]

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