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

Jonas Devlieghere via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Fri Aug 23 09:11:09 PDT 2019

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?




More information about the lldb-commits mailing list