Avoid heap allocations in IPAddress

net/base/ip_address.h

 // Copyright (c) 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef NET_BASE_IP_ADDRESS_H_
 #define NET_BASE_IP_ADDRESS_H_
 
 #include <stddef.h>
 #include <stdint.h>
 
+#include <array>
 #include <string>
 #include <vector>
 
 #include "base/compiler_specific.h"
 #include "base/strings/string_piece.h"
 #include "net/base/net_export.h"
 
 namespace net {
 
 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();
 
   // Copies the input address to |ip_address_|. The input is expected to be in
   // network byte order.
   explicit IPAddress(const std::vector<uint8_t>& address);
 
   IPAddress(const IPAddress& other);
 
   // Copies the input address to |ip_address_|. The input is expected to be in
@@ skipping 61 matching lines @@
   // Returns the canonical string representation of an IP address.
   // For example: "192.168.0.1" or "::1". Returns the empty string when
   // |ip_address_| is invalid.
   std::string ToString() const;
 
   // Parses an IP address literal (either IPv4 or IPv6) to its numeric value.
   // Returns true on success and fills |ip_address_| with the numeric value.
   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();
 
   // Returns an IPAddress instance representing the ::1 address.
   static IPAddress IPv6Localhost();
 
   // Returns an IPAddress made up of |num_zero_bytes| zeros.
   static IPAddress AllZeros(size_t num_zero_bytes);
 
   // Returns an IPAddress instance representing the 0.0.0.0 address.
   static IPAddress IPv4AllZeros();
 
   // Returns an IPAddress instance representing the :: address.
   static IPAddress IPv6AllZeros();
 
   bool operator==(const IPAddress& that) const;
   bool operator!=(const IPAddress& that) const;
   bool operator<(const IPAddress& that) const;
 
  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
 // port. For example: "192.168.0.1:99" or "[::1]:80".
 NET_EXPORT std::string IPAddressToStringWithPort(const IPAddress& address,
                                                  uint16_t port);
 
 // Returns the address as a sequence of bytes in network-byte-order.
 NET_EXPORT std::string IPAddressToPackedString(const IPAddress& address);
 
@@ skipping 54 matching lines @@
 template <size_t N>
 bool IPAddressStartsWith(const IPAddress& address, const uint8_t (&prefix)[N]) {
   if (address.size() < N)
     return false;
   return std::equal(prefix, prefix + N, address.bytes().begin());
 }
 
 }  // namespace net
 
 #endif  // NET_BASE_IP_ADDRESS_H_