[Lldb-commits] [PATCH] D61833: Fix IPv6 support on lldb-server platform

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

The actual change looks obviously correct. Below are my comments on the test situation.

> I wanted to reuse the CreateConnectedSockets part of the SocketTests. Initially I thought about creating a few functions in a test socket utility file, but then I realized the tests need to initialize the Socket plugin for these functions to work. In the end I created a new class (SocketBasedTest) so I can ensure this plugin is correctly initialized and teared down. However, I'm not a fan of subclassing as a means of code shared (as I prefer composition or delegation). What are your thoughts on this?

I don't think that the necessity of the Socket::Initialize call should prevent you from creating free functions. Everything else in lldb dealing with sockets already assumes that Socket::Initialize has been called, so I don't see why this couldn't be an implicit prerequisite of these functions (or you can even document it explicitly if you want). Though I generally also prefer free functions, I'm fairly ambivalent in this case, so I'll leave it up to you whether you want to leave this as a separate class, or convert it into a bunch of free functions. (If you do leave it as-is, I think you should at least remove the `setUp/tearDown overrides in the derived classes as they're pretty useless now - you're just overriding the methods with identical implementations.).

> Another question I have (and this is more a n00b question as I don't have prior c++ experience), the CreateConnectedSockets functions that exist use unique pointers, and after reading the documentation about this I decided to use .release() so I could safely pass them to the ConnectionFileDescriptor in the test I created for it. Should I have changed the unique pointers to shared pointers instead?

Using `release` is the correct thing to do here as ConnectionFileDescriptor takes ownership of the passed-in object. What would be a nice improvement here would be to change the interface of ConnectionFileDescriptor constructor so that it takes a `unique_ptr` instead of a raw pointer. That way, you would just say `ConnectionFileDescriptor(std::move(whatever_up))`. That way the ownership part would be explicitly documented (now, I've had to look at the source to check the ownership semantics).  If you want to do that please make a separate patch for that.



================
Comment at: lldb/unittests/Host/ConnectionFileDescriptorTest.cpp:40
+    EXPECT_TRUE(UriParser::Parse(connection_file_descriptor.GetURI(), scheme, hostname, port, path));
+    EXPECT_STREQ(ip.c_str(), hostname.str().c_str());
+    EXPECT_EQ(socket->GetRemotePortNumber(), port);
----------------
You can just do `EXPECT_EQ(ip, hostname)`


================
Comment at: lldb/unittests/Host/SocketBasedTest.cpp:63-68
+  // Return false if the host doesn't support this interface.
+  auto addresses = lldb_private::SocketAddress::GetAddressInfo(
+      listen_remote_ip.c_str(), NULL, AF_UNSPEC, SOCK_STREAM, IPPROTO_TCP);
+  if (addresses.size() == 0) {
+    return false;
+  }
----------------
The fact that you felt the need to place `// this does foo` comments everywhere is a signal that this behavior is unintuitive. I think it would be better to split this part into a separate function, and name it something like `IsAddressFamilySupported`. Then each test can do something like:
```
if (!IsAddressFamilySupported(...)) {
  GTEST_LOG_(WARNING) << "Skipping test due to missing ??? support.";
  return;
}


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61833