[Lldb-commits] [PATCH] D111964: [lldb] [lldb-gdbserver] Unify listen/connect code to use ConnectionFileDescriptor

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I like this. For lack of a better place, I'd place the `LLGSArgToURL` function into `GDBRemoteCommunicationServerLLGS.h`. That is the ~highest level file that can still be unit tested.



================
Comment at: lldb/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h:46
+      llvm::StringRef url,
+      std::function<void(llvm::StringRef local_socket_id)> socket_id_callback,
+      Status *error_ptr);
----------------
I'd use `llvm::function_ref` here


================
Comment at: lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp:550-554
+  if (!error.Fail())
+    socket_id_callback(socket_name);
+
+  if (!error.Fail())
+    error = listen_socket->Accept(socket);
----------------
you can merge these two


================
Comment at: lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp:583-610
+ConnectionStatus ConnectionFileDescriptor::UnixAbstractSocketAccept(
+    llvm::StringRef socket_name,
+    std::function<void(llvm::StringRef local_socket_id)> socket_id_callback,
+    Status *error_ptr) {
+  Status error;
+  std::unique_ptr<Socket> listen_socket = Socket::Create(
+      Socket::ProtocolUnixAbstract, m_child_processes_inherit, error);
----------------
This is identical to `NamedSocketAccept` modulo the socket type constant, right? Can you merge these?


================
Comment at: lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp:647
 
+  uint16_t port = listening_socket.get()->GetLocalPortNumber();
+  socket_id_callback(port != 0 ? std::to_string(port) : "");
----------------
Can this ever be zero ?


================
Comment at: lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp:880
+  if (llvm::Optional<URI> url = URI::Parse(url_arg)) {
+    if (!reverse_connect) {
+      // Translate the scheme from LLGS notation to ConnectionFileDescriptor.
----------------
I'd "reverse" this condition :P


================
Comment at: lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp:898
+  // expect the remainder to be the port.
+  if (host_port[0] == ':')
+    host_port = std::string("localhost") + host_port;
----------------
Can this string be empty? Maybe use `url_arg.startswith()`, just in case.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111964/new/

https://reviews.llvm.org/D111964