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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
