[Lldb-commits] [PATCH] D31823: Update LLDB Host to support IPv6 over TCP

[Zachary Turner via Phabricator via lldb-commits]

zturner added inline comments.


================
Comment at: include/lldb/Host/common/TCPSocket.h:55
+
+  std::map<int, SocketAddress> m_listen_sockets;
 };
----------------
jasonmolenda wrote:
> zturner wrote:
> > Any particular reason you're using a `std::map` instead of a `DenseMap` or other similar LLVM structure?  I was under the impression that the number of times where you should actually use `std::map` are pretty small.
> llvm docs say, "Also, because DenseMap allocates space for a large number of key/value pairs (it starts with 64 by default), it will waste a lot of space if your keys or values are large."  I think std::map is the right choice.  When it's a tossup between an llvm container class and an STL container class, my opinion is always to go with the standard container class that everyone is familiar with already.  If there's a distinct benefit to a given llvm container class for a specific scenario, that's fine, but it just increases the barrier to entry for people outside the llvm community to use these classes pervasively.
I was under the impression `SocketAddress` was fairly small, but I checked and it turns out it's not.  That said, I *still* don't think `std::map` is the right choice, because there is `std::unordered_map` which is more efficient when iterating over keys in sorted order is not a requirement.

In fact, I'm not even sure any kind of map is needed.  Isn't the point of this just to allow listening on an IPv4 interface and an IPv6 interface at the same time?  If so, the most common use case is either going to be 1 item in the map or 2 items in the map, in which case perhaps a `SmallVector<std::pair<int, SocketAddress>, 2>` is better.


https://reviews.llvm.org/D31823