[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 a comment.

Looking pretty good, I went over it again and found a few more things.  There's a bit more `auto` than I'm comfortable with, but I'm not gonna make a big deal about it.  it does make the code a bit hard to read when it's used for trivial return values though.



================
Comment at: tools/lldb-vscode/BreakpointBase.cpp:22
+  uint64_t hitCount = 0;
+  if (!llvm::StringRef(hitCondition).getAsInteger(0, hitCount))
+    bp.SetIgnoreCount(hitCount - 1);
----------------
I remembered that we have a function that wraps this and returns a more intuitive error value (true means it succeeded).  So this can actually be 

```
if (llvm::to_integer(hitCondition, hitCount))
  bp.SetIgnoreCount(hitCount-1);
```


================
Comment at: tools/lldb-vscode/BreakpointBase.h:10-11
+
+#ifndef LLDBVSCODE_BreakpointBase_h_
+#define LLDBVSCODE_BreakpointBase_h_
+
----------------
We should use all uppercase for the include guards.


================
Comment at: tools/lldb-vscode/ExceptionBreakpoint.h:21
+  lldb::LanguageType language;
+  bool value;
+  lldb::SBBreakpoint bp;
----------------
What is `value`?  Can you maybe make a more descriptive name?  Does it indicate whether the breakpoint is enabled?


================
Comment at: tools/lldb-vscode/ExceptionBreakpoint.h:24-25
+  ExceptionBreakpoint(std::string f, std::string l, lldb::LanguageType lang) :
+    filter(std::move(f)),
+    label(std::move(l)),
+    language(lang),
----------------
This doesn't look clang-formatted, I would expect some of the constructor initializers to be on the same line.


================
Comment at: tools/lldb-vscode/JSONUtils.cpp:93
+  std::vector<std::string> strs;
+  if (auto json_array = obj->getArray(key)) {
+    for (const auto &value : *json_array) {
----------------
Can we do an early return here?  Also I haven't checked, but if the `json_array` object contains a `size()` field, which I expect it does, then we might want to do a `reserve()` on the vector before adding items to it.


================
Comment at: tools/lldb-vscode/JSONUtils.cpp:97-98
+      case llvm::json::Value::String:
+        if (auto s = value.getAsString())
+          strs.push_back(s->str());
+        break;
----------------
I don't think we need the if statement here.  Unless `value.kind()` has a bug in it, this should be guaranteed to be correct without the check.


================
Comment at: tools/lldb-vscode/JSONUtils.cpp:142
+        char str[64];
+        snprintf(str, sizeof(str), "0x%" PRIx64, address);
+        type_and_addr += str;
----------------
I'd like to avoid using unsafe functions from the `printf` family if possible.  I think this function could be written:

```
StringRef Value = v.GetValue();
StringRef Summary = v.GetSummary();
StringRef TypeName = v.GetType().GetDisplayTypeName();

std::string Result;
llvm::raw_string_ostream OS(Result);
if (!Value.empty()) {
  OS << Value;
  if (!Summary.empty())
    OS << ' ' << Summary;
} else if (!Summary.empty()) {
  OS << ' ' << Summary;
} else if (!TypeName.empty()) {
  OS << TypeName;
  lldb::addr_t address = v.GetLoadAddress();
  if (address != LLDB_INVALID_ADDRESS)
    OS << " @ " << llvm::format_hex(address, 0);
}
object.try_emplace(OS.str());
```

I think this is a lot more concise.


================
Comment at: tools/lldb-vscode/JSONUtils.cpp:287
+  llvm::json::Object object;
+  if (bp_loc.IsValid()) {
+    object.try_emplace("verified", true);
----------------
Can we do an early return here?


================
Comment at: tools/lldb-vscode/JSONUtils.cpp:305
+void AppendBreakpoint(lldb::SBBreakpoint &bp, llvm::json::Array &breakpoints) {
+  if (bp.IsValid()) {
+    const auto num_locations = bp.GetNumLocations();
----------------
Early return here.


================
Comment at: tools/lldb-vscode/JSONUtils.cpp:307
+    const auto num_locations = bp.GetNumLocations();
+    if (num_locations > 0) {
+      for (size_t i = 0; i < num_locations; ++i) {
----------------
And here.


================
Comment at: tools/lldb-vscode/JSONUtils.cpp:471-472
+      object.try_emplace("name", name);
+    char path[PATH_MAX] = "";
+    file.GetPath(path, sizeof(path));
+    if (path[0]) {
----------------
Can we use the `std::string` overload here so we aren't limited to `PATH_MAX`?


================
Comment at: tools/lldb-vscode/JSONUtils.cpp:541-543
+        snprintf(str, sizeof(str),
+                 "0x%8.8" PRIx64 ": <%+" PRIi64 ">%*s %12s %s", inst_addr,
+                 inst_offset, spaces, "", m, o);
----------------
Can we use a `raw_string_ostream` here and non-printf formatting functions here and elsewhere in this function?  One way to re-write this would be:

```
Stream << formatv("{0:X+}: <{1}> {2} {3} {4}", inst_addr, inst_offset, fmt_repeat(' ', spaces), m, o);
```

Which I think is nice and easy to read.  Similarly for the rest of the function.


================
Comment at: tools/lldb-vscode/JSONUtils.cpp:548-549
+                 inst_offset, spaces, "", m, o);
+      std::string line;
+      line = str;
+      const uint32_t comment_row = 60;
----------------
With the above approach you don't need this copy because you're rendering it directly to the string you ultimately plan to use anyway, so this could go away.  Also not a huge deal, but if you pull this `std::string` out of the for loop, you can avoid a re-allocation on every iteration.


================
Comment at: tools/lldb-vscode/JSONUtils.cpp:559
+      source.content.append(line);
+      source.content.append(1, '\n');
+      source.addr_to_line[inst_addr] = i + 1;
----------------
Maybe just `source.content.push_back('\n');'


================
Comment at: tools/lldb-vscode/JSONUtils.h:17
+#include "VSCodeForward.h"
+
+//------------------------------------------------------------------
----------------
Can you put all the functions in this file in a namespace?  I realize this is a standalone utility but it's still good practice to not pollute the global namespace from a header file.


================
Comment at: tools/lldb-vscode/JSONUtils.h:25
+/// @return
+///     A std::string that contains the string value, or an empty
+///     string if \a value isn't a string.
----------------
Comment should be updated.


================
Comment at: tools/lldb-vscode/JSONUtils.h:41
+/// @return
+///     A std::string that contains the string value for the
+///     specified \a key, or an empty string if there is no key that
----------------
Commented should be updated here too.


================
Comment at: tools/lldb-vscode/LLDBUtils.cpp:15
+                     const std::vector<std::string> &commands,
+                     lldb::SBStream &strm) {
+  if (commands.empty())
----------------
Can this be an `llvm::raw_ostream&` instead of an `SBStream&`?


https://reviews.llvm.org/D50365