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

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added a comment.

@davide @jingham Thank you for the review, those were good catches and comments.

I have addressed them except for refactoring the test to use lldbInline which I will be working on now.



================
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
----------------
davide wrote:
> 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).
This looks like a good idea, I will try it out and let you know.


================
Comment at: source/Plugins/Language/CPlusPlus/LibCxxOptional.cpp:63
+  if (!val_sp) {
+      fprintf( stderr, "no __val_!\n" ) ;
+    return ValueObjectSP();
----------------
davide wrote:
> jingham wrote:
> > 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.
> if you want to log diagnostics, you might consider using the `LOG` facility and then add these to the `data formatters` channel.
Thank you for catching, I meant to remove this.


https://reviews.llvm.org/D49271