Refactor TestClient to use std::unique_ptr, and fix VirtualSocketServerTest leaks.

webrtc/base/testclient_unittest.cc

Index: webrtc/base/testclient_unittest.cc
diff --git a/webrtc/base/testclient_unittest.cc b/webrtc/base/testclient_unittest.cc
index 55078134c3a2b4ad7bc1fb5b873029ae97586b95..edebd5c95e0c3e82d8711b35c09b531898610aec 100644
--- a/webrtc/base/testclient_unittest.cc
+++ b/webrtc/base/testclient_unittest.cc
@@ -8,10 +8,11 @@
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
+#include "webrtc/base/testclient.h"
 #include "webrtc/base/gunit.h"
 #include "webrtc/base/nethelpers.h"
 #include "webrtc/base/physicalsocketserver.h"
-#include "webrtc/base/testclient.h"
+#include "webrtc/base/ptr_util.h"
 #include "webrtc/base/testechoserver.h"
 #include "webrtc/base/thread.h"
 
@@ -23,7 +24,7 @@ void TestUdpInternal(const SocketAddress& loopback) {
       ->CreateAsyncSocket(loopback.family(), SOCK_DGRAM);
   socket->Bind(loopback);
 
-  TestClient client(new AsyncUDPSocket(socket));
+  TestClient client(MakeUnique<AsyncUDPSocket>(socket));
   SocketAddress addr = client.address(), from;
   EXPECT_EQ(3, client.SendTo("foo", 3, addr));
   EXPECT_TRUE(client.CheckNextPacket("foo", 3, &from));
@@ -37,11 +38,11 @@ void TestTcpInternal(const SocketAddress& loopback) {
 
   AsyncSocket* socket = main->socketserver()
       ->CreateAsyncSocket(loopback.family(), SOCK_STREAM);
-  AsyncTCPSocket* tcp_socket = AsyncTCPSocket::Create(
-      socket, loopback, server.address());
+  std::unique_ptr<AsyncTCPSocket> tcp_socket(
+      AsyncTCPSocket::Create(socket, loopback, server.address()));

[kwiberg-webrtc, 2017/05/05 11:04:42]

Hmm. Maybe

  auto tcp_socket = WrapUnique(
      AsyncTCPSocket::Create(socket, loopback, server.address()));

WrapUnique has two big advantages over calling the unique_ptr constructor: you
don't have to mention the type, and the call will only work with a raw pointer
argument (so that whoever reads the code doesn't have to wonder if the argument
is a raw pointer or a unique_ptr).

[nisse-webrtc, 2017/05/05 12:12:56]

Not mentioning the type is a mixed blessing, though... But in this case, maybe
it's fairly clear from other context.

But when AsyncTCPSocket::Create is refactored to return a unique_ptr (like it
should), we'd delete WrapUnique here, and then with auto there's no local
context left saying if we're working with a smart pointer or not. I'm feeling
we're getting close to intercal PLEASE territory.

And there's no local guarantee that a method which happens to be named
Foo::Create actually returns a Foo or a Foo pointer.

For now, I think I prefer to not use auto.
Makes some sense. I've picked up the pattern

  std::unique_ptr<Foo> p(some raw Foo pointer);

from other places in webtrc. You're saying that we should generally stop writing
this, and instead use = WrapUnique? (I've read some posting on WrapUnique, but I
probably didn't get all the details).

[kwiberg-webrtc, 2017/05/05 12:40:31]

Yes. I'd say it's a justifiable leap of faith to assume that Foo::Create returns
a Foo, or possibly a subclass of Foo.
Yes, ideally we'd be able to say

  unique_ptr<auto> x = Foo::Create();

in such cases, but we can't, so I agree that naming the complete type is the
least bad option in such cases.
I would argue that if it does return something other than a Foo pointer (or a
subclass), then there's a naming bug.
Fair enough. There are good arguments on both sides.
Yes, for cases where MakeUnique isn't an option, I think

  std::unique_ptr<Foo> p = WrapUnique(x);

is strictly better than

  std::unique_ptr<Foo> p(x);

It's more characters, but it clearly highlights a point where an owning raw
pointer is converted to a unique_ptr. Every time you see a call to WrapUnique
you know that an owning raw pointer is being converted to a unique_ptr, because
that's the only thing it can do.

   ASSERT_TRUE(tcp_socket != nullptr);
 
-  TestClient client(tcp_socket);
+  TestClient client(std::move(tcp_socket));
   SocketAddress addr = client.address(), from;
   EXPECT_TRUE(client.CheckConnected());
   EXPECT_EQ(3, client.Send("foo", 3));