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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
