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



_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to