labath added a comment.
Ok, so the comments below are basically a collection of nits. The only two
major issues that still need addressing are:
- getting rid of the sleep in the startup code
- deciding on the naming of members
================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:23
+ auto elements_or_error = SplitPairList("ProcessInfo", response);
+ if (auto split_error = elements_or_error.takeError()) {
+ return std::move(split_error);
----------------
`if (!elements_or_error) return elements_or_error.takeError()` is a bit shorter
================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:27
+
+ auto elements = *elements_or_error;
+ if (elements["pid"].getAsInteger(16, process_info.pid))
----------------
This introduces a copy. It does not matter much for test code, but best be wary
of that issue in general. You should do `auto &elements = ...`
================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:47
+ ThreadInfo() = default;
+ explicit ThreadInfo(llvm::StringRef name, llvm::StringRef reason,
+ const RegisterMap ®isters, unsigned int signal);
----------------
explicit not needed
================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:50
+
+ std::string ReadRegister(unsigned int register_id) const;
+ bool ReadRegisterAsUint64(unsigned int register_id, uint64_t &value) const;
----------------
StringRef
================
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:22
+#include <future>
+#include <iostream>
+#include <sstream>
----------------
Including iostream is banned.
Besides, I'm pretty sure you don't need it for what you are doing in this file.
================
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:60
+ GTEST_LOG_(ERROR) << formatv("Failure to launch lldb server: {0}.",
+ status.AsCString())
+ .str();
----------------
Status has a format provider, so you can just drop the `.AsCString()` thingy
here.
================
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:124
+ if (!SendMessage("jThreadsInfo", response))
+ return jthreads_info;
+ auto creation = JThreadsInfo::Create(response, process_info->GetEndian());
----------------
`return llvm::None;` Then you can drop the dummy optional object.
================
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:131
+
+ return *creation;
+ // jthreads_info = *creation;
----------------
std::move(*creation) to avoid a copy
================
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:137
+const StopReply &TestClient::GetLatestStopReply() {
+ return (stop_reply.getValue());
+}
----------------
extra parens
================
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:216
+
+ stop_reply = *creation;
+ return true;
----------------
std::move
================
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:222
+ StartListenThread(g_local_host, 0);
+ auto connection = (ConnectionFileDescriptor *)GetConnection();
+ uint16_t listening_port = connection->GetListeningPort(UINT32_MAX);
----------------
static_cast<CFD*>
https://reviews.llvm.org/D32930
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
