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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

The patch looks fine, modulo some inline nits. I am somewhat doubtful about the name of the new function though. When I first saw `GetConnectURI`, i thought this would be the URI used to connect to `this` socket, but in reality it is the URI of the other end (right?). In that case, I think `GetRemoteURI` would be better. Or maybe, borrowing from the BSD socket vocabulary, `GetPeerURI` ? WDYT?

In D62089#1507283 <https://reviews.llvm.org/D62089#1507283>, @aadsm wrote:

> I'm not really sure which source version lldb-server is being shipped with Android Studio today (since their version works just fine obviously)...


I'm not sure either, but I'd expect it's a pretty old one (>1yr) at this point. I guess this behavior regressed somehow since then..



================
Comment at: lldb/source/Host/posix/DomainSocket.cpp:147-151
+    StreamString strm;
+    strm.Printf("%s://%s",
+                GetNameOffset() == 0 ? "unix-connect" : "unix-abstract-connect",
+                GetSocketName().c_str());
+    return strm.GetString();
----------------
You could just write something like `llvm::formatv("{0}://{1}", GetNameOffset() == 0  ? ... , GetSocketName())`


================
Comment at: lldb/unittests/Host/SocketTest.cpp:165-166
+  llvm::StringRef path;
+  EXPECT_TRUE(UriParser::Parse(socket_a_up->GetConnectURI(), scheme, hostname,
+                               port, path));
+  EXPECT_STREQ(scheme.str().c_str(), "connect");
----------------
The same issue as in the previous patch. You'll need to store the GetConnectURI result in a local variable.


================
Comment at: lldb/unittests/Host/SocketTest.cpp:167
+                               port, path));
+  EXPECT_STREQ(scheme.str().c_str(), "connect");
+  EXPECT_EQ(port, socket_a_up->GetRemotePortNumber());
----------------
`EXPECT_EQ("connect", scheme);` is enough.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62089