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

[Zachary Turner via Phabricator via lldb-commits]

zturner added a comment.

Getting pretty close.  Sorry, still have a few more nits.



================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:25-30
+  elements["pid"].getAsInteger(16, pid);
+  elements["parent-pid"].getAsInteger(16, parent_pid);
+  elements["real-uid"].getAsInteger(16, real_uid);
+  elements["real-gid"].getAsInteger(16, real_gid);
+  elements["effective-uid"].getAsInteger(16, effective_uid);
+  elements["effective-gid"].getAsInteger(16, effective_gid);
----------------
Since this is in a constructor, what happens if you get a malformed response?  You don't have a way to return an error here, and none of these errors are checked.  Do you want to `assert` if this happens?  Or are you ok silently dropping this kind of failure?


================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:109
+      if (value_str.getAsInteger(16, register_value)) {
+        return std::shared_ptr<JThreadsInfo>(nullptr);
+      }
----------------
You don't need to explicitly construct a `std::shared_ptr`.  You can just write `return nullptr;`  There are a couple of other places above this that do the same thing.


================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:142
+    llvm::support::endianness endian) {
+  auto stop_reply = std::shared_ptr<StopReply>(new StopReply());
+
----------------
Can you use `llvm::make_shared<StopReply>()` here?  Probably doesn't matter much, but it's good practice to use `make_shared<>` wherever possible since it uses less memory.


================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:159-160
+    unsigned long pc;
+    threads[i].getAsInteger(16, thread_id);
+    pcs[i].getAsInteger(16, pc);
+    stop_reply->thread_pcs[thread_id] = pc;
----------------
Is Error checking needed on these two calls?


================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:167
+    std::stringstream ss;
+    ss << std::hex << std::setw(2) << std::setfill('0') << register_id;
+    std::string hex_id = ss.str();
----------------
Is this the same as `std::string hex_id = llvm::utostr(register_id);`?


================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:189
+    if (key.size() >= 9 && key[0] == 'T' && key.substr(3, 6) == "thread") {
+      val.getAsInteger(16, stop_reply->thread);
+      key.substr(1, 2).getAsInteger(16, stop_reply->signal);
----------------
Is error checking needed here?


================
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:105-108
+  std::stringstream message;
+  message << "vCont;c:" << std::hex << thread_id;
+  std::string response;
+  SendMessage(message.str(), response);
----------------
Can you use `llvm` formatting methods instead of `std::stringstream` here?

`std::string message = llvm::formatv("vCont;c:{0:x-}", thread_id).str();`


================
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:155-156
+  for (unsigned int register_id = 0; ; register_id++) {
+    std::stringstream message;
+    message << "qRegisterInfo" << std::hex << register_id;
+    // This will throw if we scan all registers and never get the 
----------------
`std::string message = llvm::formatv("qRegisterInfo{0:x-}", register_id).str();`


https://reviews.llvm.org/D32930