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

Zachary Turner via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Fri May 12 20:17:11 PDT 2017

zturner added inline comments.

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);
jmajors wrote:
> 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.
You'll want to use something like I've got in D33059 for checking the values of `Expected<T>`s and `Error`s.  That's pending code review though.  You can put the logic in the body as you suggested, but it's really cumbersome.

Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:26-27
+Expected<std::unique_ptr<ProcessInfo>> ProcessInfo::Create(StringRef response) {
+  std::unique_ptr<ProcessInfo> process_info =
+    std::unique_ptr<ProcessInfo>(new ProcessInfo);
+  auto elements = SplitPairList(response);
How about `std::unique_ptr<ProcessInfo> process_info(new ProcessInfo);`


More information about the lldb-commits mailing list