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