labath added a comment.
I think we're getting close, but I see a couple more issues here.
================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:24
+ if (elements["pid"].getAsInteger(16, process_info->pid))
+ return make_parsing_error("ProcessInfo: pid");
+ if (elements["parent-pid"].getAsInteger(16, process_info->parent_pid))
----------------
I like how clean this ended up looking :)
================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:82
+ if (!thread_info)
+ return make_parsing_error(
+ formatv("JThreadsInfo: JSON obj at {0}", i).str());
----------------
What do you think hiding the `formatv` call inside `make_parsing_error` so we
can write `return make_parsing_error("JSON object at {0}", i);`
================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:10
+
+#include <memory>
+#include <string>
----------------
This still looks wrong. Did you run clang-format on the full patch (`git
clang-format origin/master` should do the trick. you may need to set the
clangFormat.binary git setting to point to a clang-format executable) ?
================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:55
+ private:
+ llvm::StringRef name;
+ llvm::StringRef reason;
----------------
As far as I can tell, this StringRef points to the JSON object created in
JThreadsInfo::Create, which is ephemeral to that function (I.e. it's a dangling
reference). StringRefs are great for function arguments, but you have to be
careful if you want to persist them. It's usually best to convert them back to
a string at that point. Either std::string or llvm::SmallString<N> if you want
to optimize (I guess N=16 would be a good value for thread names).
Same goes for other StringRefs persisted as member variables.
================
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:62
+ .str();
+ GTEST_LOG_(ERROR) << error;
+ return false;
----------------
This won't actually fail the test (i'm not sure whether you intended that or
not). I think it would be better to bubble the error one more level and do the
assert in the TEST. After zachary is done with D33059, we will have a nice way
to assert on llvm::Error types. After I commit D33241, you can easily convert
Status into llvm::Error.
Also the whole `Error` class is marked as nodiscard, so you won't need to
annotate all functions with the macro explicitly.
================
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:115
+
+ if (thread_id == 0) thread_id = process_info->GetPid();
+
----------------
This is a linux-ism. Other targets don't have the "pid == main thread id"
concept. What is the semantics you intended for the thread_id = 0 case? If you
wanted to resume the whole process (all threads) you can send `vCont;c` or just
`c`. We also have the LLDB_INVALID_THREAD_ID symbolic constant to signify
invalid thread.
================
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:148
+
+bool TestClient::SendMessage(const std::string& message) {
+ std::string dummy_string;
----------------
The message argument could be a StringRef to give more flexibility to the
callers. Here we don't have to worry about lifetime, as the function does not
persist the message.
================
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:223
+ << '-' << arch.GetArchitectureName() << ".log";
+ log_file.str();
+ return log_file_name;
----------------
return log_file.str()
https://reviews.llvm.org/D32930
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
