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

[Zachary Turner via Phabricator via lldb-commits]

zturner added inline comments.


================
Comment at: tools/lldb-vscode/JSONUtils.h:142
+std::vector<std::string> GetStrings(const llvm::json::Object *obj,
+                                    llvm::StringRef key);
+
----------------
clayborg wrote:
> 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 ]
> ```
> 
That's unfortunate.  All json values are backed by text in the underlying file, so in theory you could have `StringRef`s referrings to the part of the file that contains the string `true`.  But it seems the JSON library doesn't preserve the full text of each value, so maybe this isn't possible.


================
Comment at: tools/lldb-vscode/SourceBreakpoint.h:24
+  // Set this breakpoint in LLDB as a new breakpoint
+  void SetBreakpoint(const char *source_path);
+};
----------------
clayborg wrote:
> 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);
> ```
> 
In that case you can use `const llvm::Twine &`.  You can pass many types of objects to it, such as `const char*`, `std::string`, `llvm::StringRef`.  Then, inside the function you can write:

```
llvm::SmallString<32> S;
StringRef NTS = source_path.toNullTerminatedStringRef(S);
```

If the original parameter was constructed with a `const char*` or `std::string`, then `toNullTerminatedStringRef` is smart enough to know that it doesn't need to make a copy, and it just returns the pointer.  If it was constructed with a `StringRef`, then it null terminates it using `S` as the backing storage and returns that pointer.  So it's the best of both worlds


================
Comment at: tools/lldb-vscode/VSCode.h:45
+
+typedef std::map<uint32_t, SourceBreakpoint> SourceBreakpointMap;
+typedef std::map<std::string, FunctionBreakpoint> FunctionBreakpointMap;
----------------
clayborg wrote:
> zturner wrote:
> > `llvm::DenseMap`
> Causes build errors when I tried switching.
What kind of build errors?  `DenseMap<uint32_t, SourceBreakpoint>` shouldn't cause any.  If `SourceBreakpoint` was the key it might, but not if it's the value.


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


================
Comment at: tools/lldb-vscode/VSCode.h:60-61
+struct VSCode {
+  FILE *in;
+  FILE *out;
+  lldb::SBDebugger debugger;
----------------
zturner wrote:
> Any reason why we have to use `FILE*` instead of fds?
I think maybe `out` would be better as an `llvm::raw_fd_ostream`, and `in` would be better as an `llvm::MemoryBuffer` which you could obtain via `llvm::MemoryBuffer::getSTDIN()`.  These are always `stdin` and `stdout` and never seem to change, and being able to stream data to `out` and read from `in` as very convenient and cleans up a lot of code.


================
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);
----------------
clayborg wrote:
> 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
That shouldn't be a problem, should it?  With the current code, if you write `GetExceptionBreakpoint("foo");` there will be an unnecessary allocation.  With `StringRef`, there won't.  I'm not aware of *any* good reason to prefer `const string&` over `StringRef` unless you need to guarantee null termination, but that is questionable as well since the implementation details of a function end up affecting its API, so the implementation details aren't truly hidden.


https://reviews.llvm.org/D50365