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