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