[Lldb-commits] [PATCH] D32930: New framework for lldb client-server communication tests.

[Pavel Labath via Phabricator via lldb-commits]

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 &registers, 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