labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.
Thank you for doing this. We probably should have done something like this a
long time ago. I have a couple of inline comments, but they're all very
trivial. Feel free to commit, if you agree with them.
================
Comment at: source/Host/common/Socket.cpp:95-96
+ } else {
+ return llvm::make_error<llvm::StringError>(
+ "WSAStartup error.", llvm::mapWindowsError(::WSAGetLastError()));
+ }
----------------
I'd make this
`llvm::errorCodeToError(llvm::mapWindowsError(::WSAGetLastError()));` as it's
somewhat shorter and hopefully produces a better error message.
================
Comment at: source/Host/common/Socket.cpp:103
+
+void Socket::Terminate(void) {
+#if defined(_WIN32)
----------------
we don't use `(void)` in functions taking no arguments.
================
Comment at: source/Initialization/SystemInitializerCommon.cpp:118-121
+ llvm::Error error = Socket::Initialize();
+ if (error)
+ return error;
+
----------------
These calls should be roughly in dependency order. Since Sockets are in `Host`,
I'd probably put this somewhere around the HostInfo::Initialize call on line
93. (same goes for the terminate call below)
================
Comment at: unittests/Host/MainLoopTest.cpp:22
static void SetUpTestCase() {
-#ifdef _MSC_VER
- WSADATA data;
- ASSERT_EQ(0, WSAStartup(MAKEWORD(2, 2), &data));
-#endif
+ ASSERT_TRUE(!Socket::Initialize().operator bool());
}
----------------
`ASSERT_THAT_ERROR(Socket::Initialize(), llvm::Succeeded())`. That way, if this
ever fails, you'll get the actual error message instead of "false is not true".
(you might need to include something to have these available).
Repository:
rLLDB LLDB
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60440/new/
https://reviews.llvm.org/D60440
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
