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

[Davide Italiano via Phabricator via lldb-commits]

davide added a comment.

Some comments. Jonas looked at many formatters recently so he's in a good position to take a look.



================
Comment at: packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional/Makefile:9
+CXXFLAGS += -std=c++17
+#CFLAGS += -std=c++17
----------------
commented out code?


================
Comment at: packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional/TestDataFormatterLibcxxOptional.py:21-22
+    @add_test_categories(["libc++"])
+##    @skipIf(debug_info="gmodules",
+##            bugnumber="https://bugs.llvm.org/show_bug.cgi?id=36048")
+    def test_with_run_command(self):
----------------
ditto


================
Comment at: packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional/main.cpp:14-38
+    optional_int number ;
+
+    printf( "%d\n", number.has_value() ) ; // break here
+
+    number = 42 ;
+
+    printf( "%d\n", *number ) ; // break here
----------------
You don't really need to se all these breakpoints.
You just need set one on the return and print all the types.
Also, an `lldbInline` python test here should suffice and it's probably more readable (grep for lldbInline).


================
Comment at: source/Plugins/Language/CPlusPlus/CMakeLists.txt:14-15
   LibCxxTuple.cpp
+  LibCxxOptional.cpp
   LibCxxUnorderedMap.cpp
   LibCxxVector.cpp
----------------
Please sort this.


================
Comment at: source/Plugins/Language/CPlusPlus/LibCxx.cpp:48-51
+  const char *engaged_as_cstring = engaged_sp->GetValueAsUnsigned(0) == 1 ? "true" : "false" ;
+
+  stream.Printf(" engaged=%s",  engaged_as_cstring );
+
----------------
Can you use StringRef?


================
Comment at: source/Plugins/Language/CPlusPlus/LibCxx.h:30-33
+bool LibcxxOptionalSummaryProvider(
+    ValueObject &valobj, Stream &stream,
+    const TypeSummaryOptions
+        &options); // libc++ std::optional<>
----------------
Please clang-format this patch.


================
Comment at: source/Plugins/Language/CPlusPlus/LibCxxOptional.cpp:59
+
+  //ValueObjectSP val_sp( m_backend.GetChildMemberWithName(ConstString("__val_"), true));
+  ValueObjectSP val_sp( m_backend.GetChildMemberWithName(ConstString("__engaged_") , true)->GetParent()->GetChildAtIndex(0,true)->GetChildMemberWithName(ConstString("__val_"), true) );
----------------
Please remove this commented out code.


================
Comment at: source/Plugins/Language/CPlusPlus/LibCxxOptional.cpp:63
+  if (!val_sp) {
+      fprintf( stderr, "no __val_!\n" ) ;
+    return ValueObjectSP();
----------------
if you want to log diagnostics, you might consider using the `LOG` facility and then add these to the `data formatters` channel.


https://reviews.llvm.org/D49271