Avoid heap allocations in IPAddress

net/base/ip_address.h

Index: net/base/ip_address.h
diff --git a/net/base/ip_address.h b/net/base/ip_address.h
index e326e78da6e08a75f97fc4647c2c7252d7bd321a..d3e49247fa75bfdc26bae281e1d2d3a9f3f626ef 100644
--- a/net/base/ip_address.h
+++ b/net/base/ip_address.h
@@ -8,6 +8,7 @@
 #include <stddef.h>
 #include <stdint.h>
 
+#include <array>
 #include <string>
 #include <vector>
 
@@ -21,6 +22,66 @@ class NET_EXPORT IPAddress {
  public:
   enum : size_t { kIPv4AddressSize = 4, kIPv6AddressSize = 16 };
 
+  // Helper class to represent the sequence of bytes in an IP address.
+  // A vector<uint8_t> would be simpler but incurs heap allocation, so
+  // IPAddressBytes uses a fixed size array
+  class IPAddressBytes {
+   public:
+    IPAddressBytes();
+    IPAddressBytes(const IPAddressBytes& other);
+    ~IPAddressBytes();
+
+    // Returns the number of elements in the underlying array.
+    size_t size() const { return size_; }
+
+    // Sets the size to be |size|. Does not actually change the size
+    // of the underlying array.
+    void resize(size_t size) {
+      DCHECK_GT(16u, size);

[eroman, 2017/05/12 23:27:02]

DCHECK_LE(size, 16);

Either way, need equality in there.

[Ryan Hamilton, 2017/05/13 13:20:47]

Done.

+      size_ = static_cast<uint8_t>(size);
+    }
+
+    // Returns true if the underlying array is empty.
+    bool empty() const { return size_ == 0; }
+
+    // Returns a pointer to the underlying array of bytes.
+    const uint8_t* data() const { return bytes_.data(); }
+    uint8_t* data() { return bytes_.data(); }
+
+    // Returns a pointer to the first element.
+    const uint8_t* begin() const { return data(); }
+
+    // Returns a pointer past the last element.
+    const uint8_t* end() const { return &(data()[size_]); }

[eroman, 2017/05/12 23:25:05]

return data() + size;

[Ryan Hamilton, 2017/05/13 13:20:47]

Done.

+
+    // Returns a reference to the last element.
+    uint8_t& back() { return bytes_[size_ - 1]; }

[eroman, 2017/05/12 23:25:05]

DCHECK(!empty())  ?

[Ryan Hamilton, 2017/05/13 13:20:47]

Done.

+    const uint8_t& back() const { return bytes_[size_ - 1]; }

[eroman, 2017/05/12 23:25:05]

ditto.

[Ryan Hamilton, 2017/05/13 13:20:47]

Done.

+
+    // Appends |val| to the end and increments the size.
+    void push_back(uint8_t val) {
+      DCHECK_GT(16, size_);
+      bytes_[size_++] = val;
+    }
+
+    // Returns a reference to the byte at index |pos|.
+    uint8_t& operator[](size_t pos) { return bytes_[pos]; }

[eroman, 2017/05/12 23:25:05]

DCHECK that it is in range?

[Ryan Hamilton, 2017/05/13 13:20:47]

Done.

+    const uint8_t& operator[](size_t pos) const { return bytes_[pos]; }
+
+   private:
+    friend bool operator>(IPAddressBytes lhs, IPAddressBytes rhs);

[eroman, 2017/05/12 23:25:05]

const IPAddressBytes&

[Ryan Hamilton, 2017/05/13 13:20:47]

Done.

+    friend bool operator<(IPAddressBytes lhs, IPAddressBytes rhs);
+    friend bool operator!=(IPAddressBytes lhs, IPAddressBytes rhs);
+    friend bool operator==(IPAddressBytes lhs, IPAddressBytes rhs);
+
+    // Number of elements in |bytes_|. Should be either kIPv4AddressSize
+    // or kIPv6AddressSize.

[eroman, 2017/05/12 23:25:05]

or 0.

[Ryan Hamilton, 2017/05/13 13:20:47]

Good point!

+    uint8_t size_;

[eroman, 2017/05/12 23:25:05]

[optional] I suggest putting this after |bytes_|. My rationale is it doesn't
hurt the structure padding, and if we have buffer overflows the first byte we
will corrupt is the size, which increases the odds of something going wrong
deterministically.

[Ryan Hamilton, 2017/05/13 13:20:47]

Ooh, clever.

+
+    // Underlying sequence of bytes
+    std::array<uint8_t, kIPv6AddressSize> bytes_;
+  };
+
   // Creates a zero-sized, invalid address.
   IPAddress();
 
@@ -102,8 +163,11 @@ class NET_EXPORT IPAddress {
   bool AssignFromIPLiteral(const base::StringPiece& ip_literal)
       WARN_UNUSED_RESULT;
 
-  // Returns the underlying byte vector.
-  const std::vector<uint8_t>& bytes() const { return ip_address_; };
+  // Returns the underlying bytes.
+  const IPAddressBytes& bytes() const { return ip_address_; };
+
+  // Returns the underlying bytes as a vector.
+  std::vector<uint8_t> BytesAsVector() const;
 
   // Returns an IPAddress instance representing the 127.0.0.1 address.
   static IPAddress IPv4Localhost();
@@ -127,11 +191,16 @@ class NET_EXPORT IPAddress {
  private:
   // IPv4 addresses will have length kIPv4AddressSize, whereas IPv6 address
   // will have length kIPv6AddressSize.
-  std::vector<uint8_t> ip_address_;
+  IPAddressBytes ip_address_;

[eroman, 2017/05/12 23:25:05]

Was IPAddressBytes made a separate class (as opposed to merging into IPAddress)
just to preserve the immutability of IPAddress (whereas IPAddressBytes is
mutable)?

I don't feel too strongly either way. I think it is fine to flatting it all into
IPAddress -- will also simplify places where we create temporary buffers just to
initialize an IPAddress (rather than being able to write directly into its
memory).

[Ryan Hamilton, 2017/05/13 13:20:47]

The mutability was part of it. I also wanted to, as much as possible, isolate
the API change from the implementation change. 

 
   // This class is copyable and assignable.
 };
 
+bool operator>(IPAddress::IPAddressBytes lhs, IPAddress::IPAddressBytes rhs);

[eroman, 2017/05/12 23:25:05]

const IPAddressBytes&, or we will create unnecessary copies.

[Ryan Hamilton, 2017/05/13 13:20:47]

Done.

+bool operator<(IPAddress::IPAddressBytes lhs, IPAddress::IPAddressBytes rhs);
+bool operator!=(IPAddress::IPAddressBytes lhs, IPAddress::IPAddressBytes rhs);
+bool operator==(IPAddress::IPAddressBytes lhs, IPAddress::IPAddressBytes rhs);
+
 using IPAddressList = std::vector<IPAddress>;
 
 // Returns the canonical string representation of an IP address along with its