bulbazord added inline comments.
================
Comment at: lldb/include/lldb/Core/Debugger.h:430-431
+ template <typename... Args>
+ bool InterruptRequested(const char *cur_func,
+ const char *formatv, Args &&... args) {
+ bool ret_val = InterruptRequested();
----------------
jingham wrote:
> bulbazord wrote:
> > I think it would make more sense to have `cur_func` and `formatv` be of
> > type `llvm::StringRef`. Concretely, if you construct a `std::string` from
> > `nullptr` (like you're doing below when you make an InterruptionReport
> > object), you will crash.
> >
> > I know the recommendation is to use `INTERRUPT_REQUESTED` instead of
> > filling this manually, but inevitably somebody will go against the advice
> > and make a mistake.
> > I think it would make more sense to have `cur_func` and `formatv` be of
> > type `llvm::StringRef`. Concretely, if you construct a `std::string` from
> > `nullptr` (like you're doing below when you make an InterruptionReport
> > object), you will crash.
> >
> > I know the recommendation is to use `INTERRUPT_REQUESTED` instead of
> > filling this manually, but inevitably somebody will go against the advice
> > and make a mistake.
>
> I don't see how I can make the formatv option a StringRef. The llvm::formatv
> API only offers a version that takes a `const char *`. Anyway, these are
> formatv strings, they are almost universally going to be const strings.
> Turning them into llvm::StringRef's and back out again to use in
> llvm::formatv seems odd.
>
> But you are right, we should protect against someone passing in a null
> pointer for the function or format to InterruptRequested. Since this is
> just logging, an assert seems overkill, I'll just add null pointer checks
> here and turn them into "UNKNOWN FUNCTION" and "Unknown message".
Oh... so `llvm::formatv` does take a `const char *`... My bad there, that makes
sense, no need to change the type of `format` then.
Otherwise, I'm satisfied with the safety checks.
================
Comment at: lldb/include/lldb/Core/Debugger.h:455-456
+ InterruptionReport(std::string function_name, std::string description) :
+ m_function_name(function_name),
+ m_description(description),
+ m_interrupt_time(std::chrono::system_clock::now()),
----------------
jingham wrote:
> bulbazord wrote:
> > To avoid some unnecessary copies
> >
> > Could also do what Ismail is suggesting.
> This is a local that is copied to an ivar and never used again. Do I really
> have to put move in there explicitly for the optimizer to know it can reuse
> the value?
Yes. I created a simple example on Godbolt: https://godbolt.org/z/G4ej76Enn
Observe that the constructor in the example invokes the `std::string` copy
constructor. Add a `std::move` and it then invokes the move constructor.
================
Comment at: lldb/include/lldb/Core/Debugger.h:465
+ InterruptionReport(std::string function_name,
+ const char *format, Args &&... args) :
+ InterruptionReport(function_name, llvm::formatv(format,
std::forward<Args>(args)...)) {}
----------------
bulbazord wrote:
> since we're passing `format` to `formatv`, I think it would make sense for
> its type to be `llvm::StringRef` up-front here.
As you've pointed out above, `formatv` takes a `const char *` so ignore this.
================
Comment at: lldb/source/Core/Debugger.cpp:1280
+ const llvm::formatv_object_base &payload) :
+ m_function_name(function_name),
+ m_interrupt_time(std::chrono::system_clock::now()),
----------------
You'll also want to `std::move(function_name)` here too, I think.
================
Comment at: lldb/source/Target/TargetList.cpp:518
"target already exists it the list");
+ UnregisterInProcessTarget(target_sp.get());
m_target_list.push_back(std::move(target_sp));
----------------
jingham wrote:
> bulbazord wrote:
> > I'm somewhat puzzled by this. Why are we unregistering the target in
> > `AddTargetInternal` when we're registering it in `CreateTargetInternal`?
> > Maybe I don't understand the intent behind
> > `{Register,Unregister}InProcessTarget`.
> The point behind this is someone says "make me a target" and then in the
> process of making the target, the Target code does something slow (e.g. look
> for external debug files) that we want to interrupt. So we need a way for
> the debugger to find the "in the process of being made" Targets.
>
> "AddTargetInternal" is the API where the target gets added to the Debugger's
> TargetList, so after that method has run we will find the target in the
> regular TargetList. But between CreateTargetInternal and AddTargetInternal,
> that target isn't stored anywhere that the Debugger knows how to query. I
> added this list to hold the targets that are in the process of getting made,
> before they get added to the Debugger's TargetList.
Gotcha, that makes sense. Thanks for explaining!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D154542/new/
https://reviews.llvm.org/D154542
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
