[Lldb-commits] [PATCH] D50365: Add a new tool named "lldb-vscode" that implements the Visual Studio Code Debug Adaptor Protocol

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.


================
Comment at: tools/lldb-vscode/BreakpointBase.cpp:21
+  return fail_value;
+}
+} // namespace
----------------
Sure thing


================
Comment at: tools/lldb-vscode/ExceptionBreakpoint.cpp:22
+
+void ExceptionBreakpoint::ClearBreakpoint() {
+  if (bp.IsValid()) {
----------------
zturner wrote:
> Is it useful to have a notion of a breakpoint object that isn't actually set?  Why not just `delete` this class instance entirely and let the caller allocate a new one when they're ready.  If we want to keep this, then I think an `llvm::Optional<SBBreakpoint>` would be a better approach, because it makes explicit for someone reading the code that not existing is a valid state.
Exceptions breakpoints get set once at the beginning of the session after the "initialized" event is sent back to the debugger. The ClearBreakpoint is just for cleanup after all is said and done.


================
Comment at: tools/lldb-vscode/ExceptionBreakpoint.h:17
+
+struct ExceptionBreakpoint {
+  std::string filter;
----------------
zturner wrote:
> Should this inherit from `BreakpointBase`?
No. It doesn't actually share anything from BreakpointBase. You would think it would, but it doesn't.


================
Comment at: tools/lldb-vscode/ExceptionBreakpoint.h:24
+  ExceptionBreakpoint(const char *f, const char *l, lldb::LanguageType lang)
+      : filter(f), label(l), language(lang), value(false), bp() {}
+
----------------
sure


================
Comment at: tools/lldb-vscode/JSONUtils.h:45
+std::string GetString(const llvm::json::Object &obj, llvm::StringRef key);
+std::string GetString(const llvm::json::Object *obj, llvm::StringRef key);
+
----------------
it keeps the code much cleaner. Some of the LLVM JSON calls return an "llvm::json::Object *" when fetching a value for a key which can be NULL. The primary reason for these functions it to keep the code clean and simple. I prefer to keep these overloads. 


================
Comment at: tools/lldb-vscode/JSONUtils.h:142
+std::vector<std::string> GetStrings(const llvm::json::Object *obj,
+                                    llvm::StringRef key);
+
----------------
Need to keep as is because as noted in the description, numbers and booleans will be converted to strings. This is in case you specify arguments for your program as:

```
"args": [ 123, "hello", true ]
```



================
Comment at: tools/lldb-vscode/SourceBreakpoint.h:24
+  // Set this breakpoint in LLDB as a new breakpoint
+  void SetBreakpoint(const char *source_path);
+};
----------------
zturner wrote:
> `StringRef source_path`
I am gonna leave this one as is because we must pass a "const char *" the API that is called inside the body of this method:

```
  lldb::SBBreakpoint lldb::SBTarget::BreakpointCreateByLocation(const char *file, uint32_t line);
```



================
Comment at: tools/lldb-vscode/VSCode.h:45
+
+typedef std::map<uint32_t, SourceBreakpoint> SourceBreakpointMap;
+typedef std::map<std::string, FunctionBreakpoint> FunctionBreakpointMap;
----------------
zturner wrote:
> `llvm::DenseMap`
Causes build errors when I tried switching.


================
Comment at: tools/lldb-vscode/VSCode.h:46
+typedef std::map<uint32_t, SourceBreakpoint> SourceBreakpointMap;
+typedef std::map<std::string, FunctionBreakpoint> FunctionBreakpointMap;
+
----------------
zturner wrote:
> `llvm::StringMap`
Doesn't work with std::string and we need the std::string to back the string content.


================
Comment at: tools/lldb-vscode/VSCode.h:83
+  std::vector<std::string> exit_commands;
+  std::vector<std::string> stop_commands;
+  lldb::tid_t focus_tid;
----------------
These are usually empty. No need to use SmallVector and force storage when we don't need it.


================
Comment at: tools/lldb-vscode/VSCode.h:97
+  int64_t GetLineForPC(int64_t sourceReference, lldb::addr_t pc) const;
+  ExceptionBreakpoint *GetExceptionBreakpoint(const std::string &filter);
+  ExceptionBreakpoint *GetExceptionBreakpoint(const lldb::break_id_t bp_id);
----------------
zturner wrote:
> `StringRef filter`
This is iterating through an array of classes and check if the member variable, which is a std::string is equal. Leaving as std::string


================
Comment at: tools/lldb-vscode/VSCode.h:104
+  //----------------------------------------------------------------------
+  void SendJSON(const std::string &json_str);
+
----------------
zturner wrote:
> `StringRef json_str`
Leaving as std::string as we need to guarantee null termination


================
Comment at: tools/lldb-vscode/lldb-vscode.cpp:71
+
+int AcceptConnection(int portno) {
+  // Accept a socket connection from any host on "portno".
----------------
zturner wrote:
> Can you make all global functions `static`?
I will add an anonymous namespace around all local functions.


================
Comment at: tools/lldb-vscode/lldb-vscode.cpp:108
+
+std::vector<const char *> MakeArgv(const std::vector<std::string> &strs) {
+  // Create and return an array of "const char *", one for each C string in
----------------
zturner wrote:
> Can this take an `ArrayRef<std::string>` instead?
yes



https://reviews.llvm.org/D50365