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

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

This looks really cool :-)

I started doing a superficial pass but I have a hard time following what everything is doing. I think it could really help if you added more structure/abstraction. Can you also run clang-format over the new files?



================
Comment at: tools/lldb-vscode/lldb-vscode.cpp:65
+
+#define VARREF_LOCALS         (int64_t)1
+#define VARREF_GLOBALS        (int64_t)2
----------------
Can we replace these with static consexprs?


================
Comment at: tools/lldb-vscode/lldb-vscode.cpp:93
+
+uint64_t string_to_unsigned(const char *s, int base, uint64_t fail_value) {
+  if (s && s[0]) {
----------------
I think you could use llvm's `to_integer` from StringExtras.h for this. 


================
Comment at: tools/lldb-vscode/lldb-vscode.cpp:117
+bool
+thread_has_stop_reason(lldb::SBThread &thread) {
+  switch (thread.GetStopReason()) {
----------------
Should this maybe become a member of (SB)Thread?


================
Comment at: tools/lldb-vscode/lldb-vscode.cpp:136
+
+enum OutputType {
+  Console,
----------------
I very much prefer enum classes to prevent collisions. It's probably not that important here but it also doesn't harm to have the values qualified with the enum name. Same for the other enums in this file. 


================
Comment at: tools/lldb-vscode/lldb-vscode.cpp:253
+
+struct BreakpointBase {
+
----------------
I'd move this into a separate header file with all the other breakpoint classes. I think this file is pretty big already and it would help readability/maintainability to split things up.


================
Comment at: tools/lldb-vscode/lldb-vscode.cpp:337
+
+struct State {
+  FILE *in;
----------------
Some newlines to group related members and methods would really improve readability in this struct.


================
Comment at: tools/lldb-vscode/lldb-vscode.cpp:350
+  std::thread event_thread;
+//  std::thread forward_input_thread;
+//  std::thread forward_output_thread;
----------------
Can we remove this?


================
Comment at: tools/lldb-vscode/lldb-vscode.cpp:472
+
+#pragma mark -- other utilities
+
----------------
Building on my previous suggestion, I'd prefer separate files over these pragmas as my editor doesn't understand them.


================
Comment at: tools/lldb-vscode/lldb-vscode.cpp:512
+const char *get_value(lldb::SBValue &v, std::string &storage) {
+  // Get the value from an SBValue that we will display in VS Code. Some
+  // values don't have C string values and C string summaries. If a value has
----------------
I'd move this up and turn it into a doxygen comment.


================
Comment at: tools/lldb-vscode/lldb-vscode.cpp:924
+                 inst_offset, spaces, "", m, o);
+      std::string line;
+      line = str;
----------------
Could be a single statement.


================
Comment at: tools/lldb-vscode/lldb-vscode.cpp:1370
+
+std::string to_string(const JSONValue::SP &object) {
+  if (object) {
----------------
Should this live in the json lib?


https://reviews.llvm.org/D50365