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

Davide Italiano via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Tue Jul 17 17:29:20 PDT 2018

davide requested changes to this revision.
davide added a comment.
This revision now requires changes to proceed.

Probably last round of comments. Thanks for your patience!

Comment at: packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional/Makefile:7
+include $(LEVEL)/Makefile.rules
+CXXFLAGS += -std=c++17
you don't need this, it's implicit (`-O0`)

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])
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).

Comment at: source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp:496
+                  "libc++ std::optional synthetic children",
+                  ConstString("^std::__(ndk)?1::optional<.+>(( )?&)?$"),
+                  stl_synth_flags, true);
I'm a little nervous about this regex, but I think it's fine and I'll let Jim take another look in case I missed something.

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(
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)


More information about the lldb-commits mailing list