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