[Lldb-commits] [PATCH] D154542: Further refinements on reporting interruption in lldb

[Jim Ingham via Phabricator via lldb-commits]

jingham 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();
----------------
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".


================
Comment at: lldb/source/API/SBFrame.cpp:57-58
 
+#include <sstream>
+
 using namespace lldb;
----------------
mib wrote:
> bulbazord wrote:
> > What is this used for?
> Is this still necessary ?
Not sure what you are asking here.  We use StackFrameSP w/o saying lldb::StackFrameSP and we use VariableList for instance rather than lldb_private::VariableList.  We could remove the using statements here but that's not how we do it in general.  In some cases we don't do `using namespace lldb` but that's mostly in files that use clang API's heavily since those conflict with some clang type names and it was good to be explicit there.  But I don't think we want to be verbose like that in the SB files.


================
Comment at: lldb/source/Target/StackFrameList.cpp:31
 #include <memory>
+#include <sstream>
 
----------------
bulbazord wrote:
> Where is this used?
Dunno, but this seems more like an orthogonal cleanup unrelated to the current patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154542/new/

https://reviews.llvm.org/D154542