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 lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits