[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 marked 23 inline comments as done.
clayborg added inline comments.


================
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:
> 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
That is just way too much work for no gain.


================
Comment at: tools/lldb-vscode/VSCode.h:45
+
+typedef std::map<uint32_t, SourceBreakpoint> SourceBreakpointMap;
+typedef std::map<std::string, FunctionBreakpoint> FunctionBreakpointMap;
----------------
zturner wrote:
> 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.
I meant to remove this. I have everything working with DenseMap, DenseSet and StringMap


================
Comment at: tools/lldb-vscode/VSCode.h:46
+typedef std::map<uint32_t, SourceBreakpoint> SourceBreakpointMap;
+typedef std::map<std::string, FunctionBreakpoint> FunctionBreakpointMap;
+
----------------
zturner wrote:
> 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.
Ditto, meant to remove this. Got everything working.


================
Comment at: tools/lldb-vscode/VSCode.h:61
+  FILE *in;
+  FILE *out;
+  lldb::SBDebugger debugger;
----------------
This can work with a port number. Rather not switch away from FILE * for now. No real benefit.


https://reviews.llvm.org/D50365