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);`
https://reviews.llvm.org/D32930
_______________________________________________
lldb-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
