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

[Jason Majors via Phabricator via lldb-commits]

jmajors marked 6 inline comments as done.
jmajors added inline comments.


================
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);
----------------
zturner wrote:
> 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?
I changed it to a Create with tests for success.


================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:142
+    llvm::support::endianness endian) {
+  auto stop_reply = std::shared_ptr<StopReply>(new StopReply());
+
----------------
zturner wrote:
> 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.
I wanted to keep my constructors protected, to force the use of Create(). make_shared doesn't work with protected constructors.


================
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();
----------------
zturner wrote:
> Is this the same as `std::string hex_id = llvm::utostr(register_id);`?
No. The register IDs are all two nibbles wide, so I need the setw & setfill (or equivalent).


https://reviews.llvm.org/D32930