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