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

[Davide Italiano via Phabricator via lldb-commits]

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 += -O0
+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)


https://reviews.llvm.org/D49271