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

[Jason Majors via Phabricator via lldb-commits]

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


================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:142
+    llvm::support::endianness endian) {
+  auto stop_reply = std::shared_ptr<StopReply>(new StopReply());
+
----------------
labath wrote:
> jmajors wrote:
> > 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.
> I raise the bet to `llvm::make_unique` ;). shared pointers lead to messy ownership semantics, we should only use them when absolutely necessary, and I don't think that is the case here.
Since I'm returning copies of the pointer container in getter functions, I think I need shared, not unique.


================
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);
----------------
labath wrote:
> zturner wrote:
> > Is error checking needed here?
> More error checking definitely needed (here and everywhere else). If I break lldb-server while working on it, I'd like to see a clear error message from the test rather than an obscure crash. This should return `Expected<StopReply>` if you don't care about copying or `Expected<std::unique_ptr<StopReply>>` if you do (*), so that the test can ASSERT that the message was received and parsed correctly and print out the error otherwise.
> 
> (*) Or the out argument idea I wrote about earlier.
I converted my pointer containers to Expected<unique_ptr<Type>>. 
I noticed that ASSERT_* doesn't fail the test, it returns, so I need to make the functions in TestClient return Error or Expected<> objects and check them in the test body.


https://reviews.llvm.org/D32930