[Lldb-commits] [PATCH] D50864: Add libc++ data formatter for std::function

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added a comment.

@jingham @vsk I believe I have addressed most of your comments



================
Comment at: source/Plugins/Language/CPlusPlus/LibCxx.cpp:58
+
+  if (process != nullptr) {
+    Status status;
----------------
jingham wrote:
> vsk wrote:
> > Please use an early exit here, and everywhere else in the function where it's possible to do so. If the process has really disappeared, there's no need to print anything. In general there's no need to present an incomplete result from this formatter.
> Data formatters can get called in the absence of a process.  For simple C structs, for instance, they might be used to look at an initialized static object in a DATA section.  For instance, I could do something like:
>  
> #include <functional>
> auto g_func = [](int x, int y) {return x + y; };
> int
> main(int argc, char ** argv)
> {
>   int ret = g_func(argc, 100);
>   return ret;
> }
> 
> and I should be able to see g_func using "target var" before running the program.  I think the only place you care about the process here is whether you should resolve a load address or not.  Target->ReadMemory will read from a running process if that target has one (but preferring data from the underlying files if that's faster.)  So I think this formatter should be able to do its job with or without a process.
> 
> But I agree with Vedant that in cases where you can't get useful information (for instance if you can't get the vtable address), then you should return false immediately and not try to present a summary.  That way instead of showing an incomplete summary, lldb will show the raw printing of the type, which is more useful than a place-holder summary string.
> 
> You already do that for the case where you can't read the process memory.  Using early return when you discover an error will make it easier to ensure you are doing that right every time you get an error.
AFAIT we can't derive useful information for the static case.

`std::function` does not have constexpr constructors so it can't be initialized during constant initialization. I confirmed in lldb that all the fields are uninitialized therefore we have no pointers to examine.


================
Comment at: source/Plugins/Language/CPlusPlus/LibCxx.cpp:136-140
+                // Given:
+                //
+                //    main::$_1::__invoke(...)
+                //
+                // We want to slice off __invoke(...) and append operator()()
----------------
jingham wrote:
> In the first bit of code, you put the explaining comment before the code that handles the condition.  This time it's in the middle of the code.  Maybe move this comment before "if (symbol_name.contains(...)"
I updated the commenting a big, hopefully it clears things up.


https://reviews.llvm.org/D50864