jmajors marked 7 inline comments as done.
jmajors added inline comments.
================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:10
+
+#include <iomanip>
+#include <sstream>
----------------
labath wrote:
> Do you still need these includes?
Yes. I'm using a stringstream to convert integer register IDs to two nibble hex
strings.
================
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);
----------------
zturner wrote:
> How about `std::unique_ptr<ProcessInfo> process_info(new ProcessInfo);`
Java brainwashing. :)
================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:10
+
+#include <memory>
+#include <string>
----------------
labath wrote:
> This still looks wrong. Did you run clang-format on the full patch (`git
> clang-format origin/master` should do the trick. you may need to set the
> clangFormat.binary git setting to point to a clang-format executable) ?
I ran clang-format -i *h *cpp. It reordered the includes.
I just ran it as a git subcommand, and it didn't change these lines.
I even tried scrambling the includes around, and that's the order it seems to
want them in.
================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:63
+ public:
+ static llvm::Expected<std::unique_ptr<JThreadsInfo>> Create(
+ llvm::StringRef response, llvm::support::endianness endian);
----------------
tberghammer wrote:
> Why do we need the unique_ptr here? Can it return llvm::Expected<ProcessInfo>
> instead? Same question for the other Create functions.
Since I don't build the JThreadsInfo, ProcessInfo, and StopReply members of
TestClient in the constructor, I'd need a default constructor, which I hid to
force use of Create(). Also, it allows me to have an uninitialized value for
these members, so I can verify some things happen in the correct order.
================
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:62
+ .str();
+ GTEST_LOG_(ERROR) << error;
+ return false;
----------------
labath wrote:
> This won't actually fail the test (i'm not sure whether you intended that or
> not). I think it would be better to bubble the error one more level and do
> the assert in the TEST. After zachary is done with D33059, we will have a
> nice way to assert on llvm::Error types. After I commit D33241, you can
> easily convert Status into llvm::Error.
>
> Also the whole `Error` class is marked as nodiscard, so you won't need to
> annotate all functions with the macro explicitly.
The false return/no discard combo causes the test to fail.
I didn't want to return an Error object, because it adds a lot of overhead.
If Zachary's assert change reduces this overhead, I can switch it.
================
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:115
+
+ if (thread_id == 0) thread_id = process_info->GetPid();
+
----------------
labath wrote:
> This is a linux-ism. Other targets don't have the "pid == main thread id"
> concept. What is the semantics you intended for the thread_id = 0 case? If
> you wanted to resume the whole process (all threads) you can send `vCont;c`
> or just `c`. We also have the LLDB_INVALID_THREAD_ID symbolic constant to
> signify invalid thread.
I was using 0 so the caller didn't have to know what the main thread id was.
https://reviews.llvm.org/D32930
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
