[Lldb-commits] [PATCH] D62089: Make ConnectionFileDescription work with all sockets

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

It looks like diff now contains the changes from D61833 <https://reviews.llvm.org/D61833> as well. It would be better to upload just the changes relative to the previous patch in the series, otherwise things get confusing about what is being changed where. It's kind of obvious here, but you can use the "parent revision" field in phabricator to indicate that one diff depends on another.

Other than that, I *think* this is fine.



================
Comment at: lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp.rej:1-23
+***************
+*** 758,770 ****
+  }
+  
+  void ConnectionFileDescriptor::InitializeSocket(Socket *socket) {
+-   assert(socket->GetSocketProtocol() == Socket::ProtocolTcp);
+-   TCPSocket *tcp_socket = static_cast<TCPSocket *>(socket);
----------------
Delete this.


================
Comment at: lldb/source/Host/posix/DomainSocket.cpp:130
+
+std::string DomainSocket::GetSocketName() const {
+  if (m_socket != kInvalidSocketValue) {
----------------
aadsm wrote:
> labath wrote:
> > This should also be  `GetRemoteSocketName` or something similar (though the word `remote` looks a bit weird when it comes to domain sockets).
> uhm.. I'm a bit skeptical on this one 😄 
> As far as I understand domain sockets don't really have endpoints, there's just a single point of consumption (the inode). Here's what I see in my head when comparing TCP to Domain sockets:
> 
> TCP:
> ```
> consumer o---------o provider
> ```
> Domain:
> ```
> consumer -----o----- provider
> ```
> While on TCP we have 2 things that we can name "remote" and "local". On Domain there's only one thing to name, so I think it would be odd calling it "remote" given there's no parallel with the tcp model.
> 
> I don't have strong feelings on this though, and I'm happy to change to land the patch.
Oops, sorry, you're totally right. I just noticed this name in drive-by and didn't stop to think about it. Please ignore this comment.


================
Comment at: lldb/unittests/Host/SocketTest.cpp.rej:1-16
+***************
+*** 7,12 ****
+  //===----------------------------------------------------------------------===//
+  
+  #include "SocketTestUtilities.h"
+  #include "gtest/gtest.h"
+  
----------------
Delete this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62089