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