clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
I am fine overall with this change because of goofy things that can happen when
we always create a target. We might think about returning an error if the user
specifies "launchCommands" or "attachCommands" if the user also includes any of
the arguments that will be ignored instead of printing to the console, see
inlined comments. I would vote for returning an error as long the the full
error string is displayed to the user and the entire string can be read and
isn't obfuscated in the launch/attach error dialog. I am not set on this and
would be interested to hear what others thing of the error vs showing something
in the debugger console
================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:627-628
+ "incompatible with 'attachCommands'.\n", arg.str().c_str());
+ }
+ }
+ // Run any pre run LLDB commands the user specified in the launch.json
----------------
We can't print to stderr or stdout since this is where the VS code DAP packets
get delivered.
We have two options here IMHO:
- deliver the warning/error stirng to the debugger console
- return an error with this string as the reason and fail the attach as long as
the error string get displayed to the user in the IDE
We can deliver this to the "Debugger Console" using:
```
std::string str;
llvm::raw_string_ostream strm(str);
strm << ...;
g_vsc.SendOutput(OutputType::Console, strm.str());
```
================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:1696-1697
+ "incompatible with 'launchCommands'.\n", arg.str().c_str());
+ }
+ }
+ g_vsc.RunPreRunCommands();
----------------
use g_vsc.SendOutput(OutputType::Console, ...) as mentioned above or return an
error. We will discuss the merits of message vs error in this comments.
================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:1558
+ if (!launchCommands.empty()) {
+ // if "launchCommands" are provided, then they are expected to make the
+ // launch happen for launch requests and they replace the normal logic that
----------------
aadsm wrote:
> clayborg wrote:
> > We need to check if any arguments are set in the launch config that
> > "launchCommands" will ignore and return an error if any of them are set. Or
> > we need to emit an error or warning to the debug console so the users can
> > know that these settings are now ignored because "launchCommands" will
> > cause them to be.
> I vote for a warning here otherwise it might break people's current setups,
> assuming the error is an indication that the launch won't happen, but we
> should totally do it.
I kind of disagree after thinking about this some more. Right now we have many
things that can be specified in the launch config that will just get ignored
and if the user doesn't check the debugger console, they won't know. It is
probably ok to have the launch fail as long as the error string shows up and is
readable to the user and is complete enough for the user to read. Thoughts?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D94997/new/
https://reviews.llvm.org/D94997
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
