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

[Jim Ingham via Phabricator via lldb-commits]

jingham requested changes to this revision.
jingham added a comment.

It seems a little odd to choose to represent this as an array with 0 elements.  I think I would be confused by that.  You can maybe choose a better name like "value" for your cloned object.  But it looks like it is also possible to make a Synthetic Child Provider that just replaces the value of the object with the value provided by the synthetic child provider.  That's done in the Python front end by saying you have no children and then providing a the "get_value" method.  It must be possible to do this from C++ as well, but it will take a little thought to figure out how that works.  But that would be the most natural way to present this, I think.



================
Comment at: packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional/TestDataFormatterLibcxxOptional.py:26-38
+        self.runCmd("file " + self.getBuildArtifact("a.out"), CURRENT_EXECUTABLE_SET)
+
+        bkpt = self.target().FindBreakpointByID(
+            lldbutil.run_break_set_by_source_regexp(
+                self, "break here"))
+
+        self.runCmd("run", RUN_SUCCEEDED)
----------------
you can do this more cleanly with:

(self.target, _, _, bkpt) = lldbutil.run_to_source_breakpoint(self, "break here", lldb.SBFileSpec("main.cpp"))


================
Comment at: packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional/TestDataFormatterLibcxxOptional.py:39-48
+        # This is the function to remove the custom formats in order to have a
+        # clean slate for the next test case.
+        def cleanup():
+            self.runCmd('type format clear', check=False)
+            self.runCmd('type summary clear', check=False)
+            self.runCmd('type filter clear', check=False)
+            self.runCmd('type synth clear', check=False)
----------------
I don't think you need this cleanup, you aren't adding any formatters, you're just testing built-in ones.


================
Comment at: source/Plugins/Language/CPlusPlus/LibCxxOptional.cpp:63
+  if (!val_sp) {
+      fprintf( stderr, "no __val_!\n" ) ;
+    return ValueObjectSP();
----------------
If you want to log this, get the formatters log channel and put the text there.  Then somebody will see this when they turn on the log.  An fprintf like this won't help anybody.


https://reviews.llvm.org/D49271