[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: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