zturner added inline comments.
================
Comment at: tools/lldb-vscode/BreakpointBase.cpp:13-21
+uint64_t string_to_unsigned(const char *s, int base, uint64_t fail_value) {
+ if (s && s[0]) {
+ char *end = nullptr;
+ uint64_t uval = strtoull(s, &end, base);
+ if (*end == '\0')
+ return uval;
+ }
----------------
Can we delete this function and use `StringRef::getAsInteger()` instead?
================
Comment at: tools/lldb-vscode/BreakpointBase.h:18
+
+struct BreakpointBase {
+
----------------
This should have a virtual destructor.
================
Comment at: tools/lldb-vscode/BreakpointBase.h:24
+ // ignored. The backend is expected to interpret the expression as needed
+ std::string hitCondition;
+ // If this attribute exists and is non-empty, the backend must not 'break'
----------------
Shouldn't this be an integer?
================
Comment at: tools/lldb-vscode/ExceptionBreakpoint.cpp:14
+void ExceptionBreakpoint::SetBreakpoint() {
+ if (!bp.IsValid()) {
+ bool catch_value = filter.find("_catch") != std::string::npos;
----------------
Early return.
================
Comment at: tools/lldb-vscode/ExceptionBreakpoint.cpp:22
+
+void ExceptionBreakpoint::ClearBreakpoint() {
+ if (bp.IsValid()) {
----------------
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.
================
Comment at: tools/lldb-vscode/ExceptionBreakpoint.cpp:23
+void ExceptionBreakpoint::ClearBreakpoint() {
+ if (bp.IsValid()) {
+ g_vsc.target.BreakpointDelete(bp.GetID());
----------------
Early return.
================
Comment at: tools/lldb-vscode/ExceptionBreakpoint.h:17
+
+struct ExceptionBreakpoint {
+ std::string filter;
----------------
Should this inherit from `BreakpointBase`?
================
Comment at: tools/lldb-vscode/ExceptionBreakpoint.h:23-24
+ lldb::SBBreakpoint bp;
+ ExceptionBreakpoint(const char *f, const char *l, lldb::LanguageType lang)
+ : filter(f), label(l), language(lang), value(false), bp() {}
+
----------------
Can you make this constructor take the first two arguments as `std::string`,
then initialize them with `filter(std::move(f))`, and `label(std::move(l))`?
The current code is guaranteed to make a copy, the suggested approach might not.
================
Comment at: tools/lldb-vscode/FunctionBreakpoint.h:17
+ // The name of the function
+ std::string name;
+
----------------
Can we call this variable `functionName` and delete the comment?
================
Comment at: tools/lldb-vscode/JSONUtils.h:27
+//------------------------------------------------------------------
+std::string GetAsString(const llvm::json::Value &value);
+
----------------
I think this could return `StringRef`
================
Comment at: tools/lldb-vscode/JSONUtils.h:44-45
+//------------------------------------------------------------------
+std::string GetString(const llvm::json::Object &obj, llvm::StringRef key);
+std::string GetString(const llvm::json::Object *obj, llvm::StringRef key);
+
----------------
I think this could return a `StringRef`. Also, it seems weird to have an
overload which takes a pointer. Can we delete that overload? I don't think it
would make sense for anyone to call this function with a null `obj`.
================
Comment at: tools/lldb-vscode/JSONUtils.h:141-142
+//------------------------------------------------------------------
+std::vector<std::string> GetStrings(const llvm::json::Object *obj,
+ llvm::StringRef key);
+
----------------
This could return a `vector<StringRef>`
================
Comment at: tools/lldb-vscode/JSONUtils.h:225
+//----------------------------------------------------------------------
+llvm::json::Object CreateEvent(const char *event_name);
+
----------------
`StringRef event_name`
================
Comment at: tools/lldb-vscode/JSONUtils.h:261-262
+//----------------------------------------------------------------------
+llvm::json::Value CreateScope(const char *name, int64_t variablesReference,
+ int64_t namedVariables, bool expensive);
+
----------------
`StringRef name`
================
Comment at: tools/lldb-vscode/SourceBreakpoint.h:24
+ // Set this breakpoint in LLDB as a new breakpoint
+ void SetBreakpoint(const char *source_path);
+};
----------------
`StringRef source_path`
================
Comment at: tools/lldb-vscode/SourceReference.h:19
+ std::string content;
+ std::map<lldb::addr_t, int64_t> addr_to_line;
+
----------------
`llvm::DenseMap`
================
Comment at: tools/lldb-vscode/VSCode.cpp:22-24
+ while (*l == '\r' || *l == '\n')
+ ++l;
+ return l[0] == '\0';
----------------
I'd write this as:
```
inline bool is_empty_line(StringRef S) {
return S.ltrim().empty();
}
```
================
Comment at: tools/lldb-vscode/VSCode.cpp:129
+
+ while (fgets(line, sizeof(line), in)) {
+ if (strncmp(line, header.data(), header.size()) == 0) {
----------------
You should check out `llvm/Support/LineIterator.h`
================
Comment at: tools/lldb-vscode/VSCode.h:45
+
+typedef std::map<uint32_t, SourceBreakpoint> SourceBreakpointMap;
+typedef std::map<std::string, FunctionBreakpoint> FunctionBreakpointMap;
----------------
`llvm::DenseMap`
================
Comment at: tools/lldb-vscode/VSCode.h:46
+typedef std::map<uint32_t, SourceBreakpoint> SourceBreakpointMap;
+typedef std::map<std::string, FunctionBreakpoint> FunctionBreakpointMap;
+
----------------
`llvm::StringMap`
================
Comment at: tools/lldb-vscode/VSCode.h:60-61
+struct VSCode {
+ FILE *in;
+ FILE *out;
+ lldb::SBDebugger debugger;
----------------
Any reason why we have to use `FILE*` instead of fds?
================
Comment at: tools/lldb-vscode/VSCode.h:72-73
+ std::thread event_thread;
+ // std::thread forward_input_thread;
+ // std::thread forward_output_thread;
+ std::unique_ptr<std::ofstream> log;
----------------
Delete commented out code
================
Comment at: tools/lldb-vscode/VSCode.h:74
+ // std::thread forward_output_thread;
+ std::unique_ptr<std::ofstream> log;
+ std::map<lldb::addr_t, int64_t> addr_to_source_ref;
----------------
`llvm::Optional<llvm::raw_fd_ostream>`?
================
Comment at: tools/lldb-vscode/VSCode.h:75-76
+ std::unique_ptr<std::ofstream> log;
+ std::map<lldb::addr_t, int64_t> addr_to_source_ref;
+ std::map<int64_t, SourceReference> source_map;
+ std::map<std::string, SourceBreakpointMap> source_breakpoints;
----------------
`llvm::DenseMap<>`
================
Comment at: tools/lldb-vscode/VSCode.h:77
+ std::map<int64_t, SourceReference> source_map;
+ std::map<std::string, SourceBreakpointMap> source_breakpoints;
+ FunctionBreakpointMap function_breakpoints;
----------------
`llvm::StringMap`
================
Comment at: tools/lldb-vscode/VSCode.h:79-83
+ std::vector<ExceptionBreakpoint> exception_breakpoints;
+ std::vector<std::string> init_commands;
+ std::vector<std::string> pre_run_commands;
+ std::vector<std::string> exit_commands;
+ std::vector<std::string> stop_commands;
----------------
`llvm::SmallVector`
================
Comment at: tools/lldb-vscode/VSCode.h:89
+ // unless we send a "thread" event to indicate the thread exited.
+ std::set<lldb::tid_t> thread_ids;
+ VSCode();
----------------
`llvm::DenseSet<>`
================
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);
----------------
`StringRef filter`
================
Comment at: tools/lldb-vscode/VSCode.h:104
+ //----------------------------------------------------------------------
+ void SendJSON(const std::string &json_str);
+
----------------
`StringRef json_str`
================
Comment at: tools/lldb-vscode/VSCode.h:114-115
+
+ void SendOutput(OutputType o, const char *output,
+ size_t output_len = SIZE_MAX);
+
----------------
`StringRef output`
================
Comment at: tools/lldb-vscode/VSCode.h:130-131
+
+ void RunLLDBCommands(const char *prefix,
+ const std::vector<std::string> &commands);
+
----------------
`StringRef prefix, ArrayRef<std::string> commands`
================
Comment at: tools/lldb-vscode/lldb-vscode.cpp:47-58
+#if defined(_WIN32)
+#define PATH_MAX MAX_PATH
+typedef int socklen_t;
+constexpr const char *dev_null_path = "nul";
+
+static bool ChangeDir(const char *path) { return SetCurrentDirectoryA(path); }
+#else
----------------
This block of code can be deleted. We have similar abstractions in either lldb
or llvm.
================
Comment at: tools/lldb-vscode/lldb-vscode.cpp:61
+typedef void (*RequestCallback)(const llvm::json::Object &command);
+typedef std::map<const char *, bool> ProgressMap;
+
----------------
`llvm::StringMap`
================
Comment at: tools/lldb-vscode/lldb-vscode.cpp:69
+
+#pragma mark-- other utilities
+
----------------
I don't believe these `pragma` statements will work on all compilers
================
Comment at: tools/lldb-vscode/lldb-vscode.cpp:71
+
+int AcceptConnection(int portno) {
+ // Accept a socket connection from any host on "portno".
----------------
Can you make all global functions `static`?
================
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
----------------
Can this take an `ArrayRef<std::string>` instead?
================
Comment at: tools/lldb-vscode/lldb-vscode.cpp:432
+
+ std::string sourceMapCommand("settings set target.source-map");
+ auto sourcePath = GetString(arguments, "sourcePath");
----------------
This should be an `llvm::raw_string_ostream` which builds up the command, then
at the end call `stream.str()` to render the final string.
https://reviews.llvm.org/D50365
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
