[Lldb-commits] [PATCH] D13754: Split Socket class into Tcp/Udp/DomainSocket subclasses.

[Pavel Labath via lldb-commits]

labath accepted this revision.
labath added a comment.

Looks good, with some random comments you can ignore.


================
Comment at: include/lldb/Host/Socket.h:55
@@ -55,1 +54,3 @@
 
+    virtual Error Connect(llvm::StringRef host_and_port);
+    virtual Error Listen(llvm::StringRef host_and_port, int backlog);
----------------
I would rename these to something more neutral like `name`, `addr`, etc. as this will not be a host+port for the domain socket implementation.

================
Comment at: source/Host/common/Socket.cpp:357
@@ -740,3 +356,3 @@
 
-uint16_t Socket::GetLocalPortNumber(const NativeSocket& socket)
+Error Socket::Connect (llvm::StringRef host_and_port)
 {
----------------
Should we make this and Accept() abstract, as all implementations override it anyway?

================
Comment at: source/Host/common/TcpSocket.cpp:254
@@ +253,3 @@
+            accept_connection = true;
+            // Since both sockets have the same descriptor, arbitrarily choose the send
+            // socket to be the owner.
----------------
This comment sounds obsolete. We should probably delete it.

================
Comment at: source/Host/posix/DomainSocket.cpp:73
@@ +72,3 @@
+
+    FileSystem::Unlink(FileSpec{name, true});
+
----------------
Not really in scope of this patch, but I must say I find blindly deleting a file like this dangerous.


http://reviews.llvm.org/D13754