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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

A bunch more pedantic comments from me.



================
Comment at: unittests/tools/lldb-server/inferior/thread_inferior.cpp:26
+  for (int i = 0; i < thread_count; i++) {
+    threads.push_back(std::thread([&delay]{while(delay);}));
+  }
----------------
Could you add a (e.g. 1 second) sleep into the loop, to avoid the threads hogging the cpu.


================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:10
+
+#include <iomanip>
+#include <sstream>
----------------
Do you still need these includes?


================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:55
+    .Case("big", support::big)
+    .Default(support::native);
+
----------------
This should probably be a parsing error instead. Having the server tell us "my endianness is `native`" is not really helpful :).


================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:92
+  StructuredData::Array* array = json->GetAsArray();
+  if (!array) {
+    return make_error<ParsingError>("JThreadsInfo: JSON array");
----------------
If you're not too attached to curly braces, you can save some space here by dropping them. They have some value when the statement is complex, but for one-line bodies like this they just add clutter.


================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:197
+  unsigned int register_id = 0;
+  while (true) {
+    std::stringstream ss;
----------------
There's no guarantee lldb-server will send the register numbers in sequence. In the current implementation it expedites general purpose registers (which also happen to be the lowest numbered registers on x86 at least, but I am not sure if that is the case on all architectures). You would be better of looping through the key-value pairs and checking whether the key is a number (that's what the real client does).


================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:199
+    std::stringstream ss;
+    ss << std::hex << std::setw(2) << std::setfill('0') << register_id;
+    std::string hex_id = ss.str();
----------------
llvm::formatv or something similar


================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:10
+
+#include <memory>
+#include <string>
----------------
System includes should go last. (If you delete the empty lines between the include blocks, clang-format will take care of the order for you).


================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:20
+
+namespace CommunicationTests {
+  class ThreadInfo;
----------------
our namespaces tend to be `snake_cased` for some reason. I'm not entirely happy with the "communication" in the name, as they test much more than just communication. I guess I'd call it `llgs_test`, even though it's not entirely correct (but it's shorter!), but I don't feel about that strongly.


================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:25
+
+class ParsingError : public llvm::ErrorInfo<ParsingError> {
+public:
----------------
If we're only going to be using this type for printing errors to the user (which seems to be the case now), we might as well use llvm::StringError (possibly with a custom factory function which would insert the "argument was invalid" stuff and such for brevity). What do you think?

I am asking that because I am not sure if all errors we encounter can be described as "parsing errors", and I wanted to avoid defining a bunch of additional error types, if we don't need it. If you see a use for that, then fine.


================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:50
+
+protected:
+  ProcessInfo();
----------------
Using protected seems to hint that this class can be inherited from, which is generally a bad idea without providing a virtual destructor. (I'm not sure why would anyone want to inherit from this)


================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:54
+private:
+  pid_t pid;
+  pid_t parent_pid;
----------------
This type will not exist on windows. We can use lldb::pid_t instead. There is no lldb::uid_t and gid_t equivalent though (lldb seems to use simply uint32_t for these).


================
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:33
+namespace CommunicationTests {
+const char* LOCALHOST = "127.0.0.1";
+
----------------
`static const char g_local_host[] = ...; ` (static because the variable is file-local, [] avoids an extra indirection and makes the variable really const, the variable name is debatable, but it definitely shouldn't be all caps).


================
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:38
+: test_name(test_name), test_case_name(test_case_name), pc_register(UINT_MAX) {
+  HostInfo::Initialize();
+}
----------------
`HostInfo::Initialize` belongs to the SetUpTestCase (or possibly SetUp) of the fixture for these tests (that's how all other of our tests handle this).


================
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:124
+const ProcessInfo& TestClient::GetProcessInfo() {
+  return *(process_info.get());
+}
----------------
return *process_info


================
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:178
+    // expected result. Is that okay? Or is there a situation where
+    // no result is valid?
+    std::string response;
----------------
We should return an error if we don't find the pc register.


https://reviews.llvm.org/D32930