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

Sun Aug 25 10:16:42 PDT 2019

jankratochvil marked 5 inline comments as done.
jankratochvil added a comment.

In D66654#1643082 <https://reviews.llvm.org/D66654#1643082>, @JDevlieghere wrote:

> How much work would it be to return an `llvm::Expected<bool>` from `Get_Impl`? I'd really prefer that over the current approach.

Done.  Although now  if (Get(...)) became confusing which I have commented in `lldb_private::FormatDropExpected()`.

BTW I haven't found how to format a table for Doxygen.  Also by searching around I have found other tables in LLDB/LLVM are also not recognized by Doxygen as tables.

================
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) {
----------------
JDevlieghere wrote:
> If returning an expected isn't feasible, we should pass at least a `Status*` instead of setting the error in the `ValueObject` directly. 
With `Expected<bool>` this is no longer applicable.

================
Comment at: lldb/source/DataFormatters/TypeCategory.cpp:303
+    if (GetTypeFormatsContainer()->Get(type_name, format_sp,
+                                       nullptr /*valobj*/)) {
       if (matching_category)
----------------
JDevlieghere wrote:
> It's really unfortunate that so many call sites now require a null pointer. Can we make it a default argument?
With `Expected<bool>` this is no longer applicable.

Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66654/new/

https://reviews.llvm.org/D66654