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

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

Looks good.  I'd recommend adding clayborg as a reviewer and giving him a couple days to comment on this if he has time.  He wrote all of this code originally.



================
Comment at: include/lldb/Host/common/TCPSocket.h:55
+
+  std::map<int, SocketAddress> m_listen_sockets;
 };
----------------
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.


================
Comment at: source/Host/common/Socket.cpp:258
         regex_match.GetMatchAtIndex(host_and_port.data(), 2, port_str)) {
+      // IPv6 addresses are wrapped in [] when specified with ports
+      if (host_str.front() == '[' && host_str.back() == ']')
----------------
Ah, I'd originally said I thought we should just tack the :portnum on to the end of the string, search backwards to find it (because the [] characters are metacharacters in unix shells).  but I see that RFC8986 says this is how things like this are specified, like http://[2001:db8:1f70::999:de8:7648:6e8]:100/ so cool with that.


https://reviews.llvm.org/D31823