[Lldb-commits] [PATCH] D60440: [lldb-server] Introduce Socket::Initialize and Terminate to simply WSASocket setup

[Pavel Labath via Phabricator via lldb-commits]

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