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

[Pavel Labath via Phabricator via lldb-commits]

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