jingham added a comment.
For the most part this is fine. There are two bits to work on:
1. I think you could do all of this work on a static std::function object from
the data section even if you don't have a process. It might be worth seeing
whether you can do that. It looks like you can make static std::function
objects...
2. I think this code would be a little easier to understand if you first stated
clearly the kinds of std::function variants that you've discovered at the
beginning of the provider. Then when you get to the section of code that
handles each of the cases, start the code with: "This covers the case...". Be
kind to two years in the future you when you will have forgotten how all this
stuff works...
================
Comment at: source/Plugins/Language/CPlusPlus/LibCxx.cpp:58
+
+ if (process != nullptr) {
+ Status status;
----------------
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.
================
Comment at: source/Plugins/Language/CPlusPlus/LibCxx.cpp:80
+ Symbol *symbol;
+
+ if (target.GetSectionLoadList().ResolveLoadAddress(
----------------
vsk wrote:
> Instead of attempting to parse symbol names, which is inherently brittle and
> complicated, why not get the base address of the callable and look up it's
> location via the symbol table? You'd lose some precision in being able to
> disambiguate lambdas vs. regular functions, but that's a great tradeoff to
> make, because the code would be simpler and more reliable.
I'm not sure I agree. There were some cases where the only way Shafik could
find the target was looking at the demangled name. There's nothing here that's
so complex that it isn't worth doing, and we can test the cases to ensure that
it keeps working. The alternative is leaving folks with no idea what their
std::function holds and no way of knowing why this works sometimes but not
others.
================
Comment at: source/Plugins/Language/CPlusPlus/LibCxx.cpp:129
+ R"(::operator\(\)\(.*\))";
+ ;
+
----------------
This ";" is not doing anything, is it?
================
Comment at: source/Plugins/Language/CPlusPlus/LibCxx.cpp:136-140
+ // Given:
+ //
+ // main::$_1::__invoke(...)
+ //
+ // We want to slice off __invoke(...) and append operator()()
----------------
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(...)"
================
Comment at: source/Plugins/Language/CPlusPlus/LibCxx.cpp:202-204
+ stream.Printf(" __f_ = %llu", member__f_pointer_value);
+
+ return true;
----------------
I don't think this is valuable. You are just reporting the value of __f_ here?
If that's all you can do, I think it's better to return false indicating that
you really couldn't make sense of this object, and showing the raw value.
https://reviews.llvm.org/D50864
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
