[Lldb-commits] [PATCH] D49271: Adding libc++ formattors for std::optional

Shafik Yaghmour via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Mon Jul 23 14:52:25 PDT 2018


shafik marked 6 inline comments as done.
shafik added a comment.

@davide One more pass



================
Comment at: packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional/TestDataFormatterLibcxxOptional.py:8
+
+lldbinline.MakeInlineTest(__file__, globals(), [no_debug_info_test])
----------------
jingham wrote:
> davide wrote:
> > I do wonder if you need a decorator to indicate that this is a libcxx only test (and skip everywhere the library isn't supported).
> Yes, you do need to mark this or you will get spurious failures at least on Windows, which turns off the libcxx category.
Switched back to the old style tests due to the .categories bug we discussed earlier.


================
Comment at: source/Plugins/Language/CPlusPlus/LibCxx.cpp:42-51
+  ValueObjectSP engaged_sp(
+      valobj_sp->GetChildMemberWithName(ConstString("__engaged_"), true));
+
+  if (!engaged_sp)
+    return false;
+
+  llvm::StringRef engaged_as_cstring(
----------------
davide wrote:
> This is really obscure to somebody who doesn't understand the type internally. Can you add a comment explaining the structure? (here or in the synthetic)
That is a good point, I am looking at the cppreference page for it and I think maybe has_value might be an improvement. 


https://reviews.llvm.org/D49271





More information about the lldb-commits mailing list