Introduce rtc::ArrayView<T>, which keeps track of an array that it doesn't own

webrtc/base/array_view.h

Index: webrtc/base/array_view.h
diff --git a/webrtc/base/array_view.h b/webrtc/base/array_view.h
new file mode 100644
index 0000000000000000000000000000000000000000..94a241038c8583cae0dea1ce0d0644570b135055
--- /dev/null
+++ b/webrtc/base/array_view.h
@@ -0,0 +1,80 @@
+/*
+ *  Copyright 2015 The WebRTC Project Authors. All rights reserved.
+ *
+ *  Use of this source code is governed by a BSD-style license
+ *  that can be found in the LICENSE file in the root of the source
+ *  tree. An additional intellectual property rights grant can be found
+ *  in the file PATENTS.  All contributing project authors may
+ *  be found in the AUTHORS file in the root of the source tree.
+ */
+
+#ifndef WEBRTC_BASE_ARRAY_VIEW_H_
+#define WEBRTC_BASE_ARRAY_VIEW_H_
+
+#include <vector>
+
+#include "webrtc/base/checks.h"
+
+namespace rtc {
+
+// Keeps track of an array (a pointer and a size) that it doesn't own.
+// ArrayView objects are immutable except for assignment, and small enough to
+// be cheaply passed by value.
+//
+// Note that ArrayView<T> and ArrayView<const T> are distinct types; this is
+// how you would represent mutable and unmutable views of an array.
+template <typename T>
+class ArrayView final {
+ public:
+  // Construct an empty ArrayView.
+  ArrayView() : ArrayView(static_cast<T*>(nullptr), 0) {}

[the sun, 2015/10/21 14:06:34]

Is the cast really necessary?

[kwiberg-webrtc, 2015/10/22 11:35:33]

Yes. I get

  candidate template ignored: could not match 'U *' against 'nullptr_t'

for the constructor that it's supposed to be choosing if I don't cast. Looks
like nullptr might in some sense not actually *be* a pointer, even though it
converts implicitly to any pointer type.

+
+  // Construct an ArrayView for a (pointer,size) pair.
+  template <typename U>
+  ArrayView(U* data, size_t size) : data_(data), size_(size) {
+    // Check invariant: !data_ iff size_ == 0.
+    RTC_DCHECK_EQ(!data_, size_ == 0);
+  }
+
+  // Construct an ArrayView for an array.
+  template <typename U, size_t N>
+  ArrayView(U (&array)[N]) : ArrayView(array + 0, N) {}

[mgraczyk, 2015/10/22 00:12:26]

Why array + 0 instead of &array[0]?

[kwiberg-webrtc, 2015/10/22 11:35:33]

It seemed simpler: it's one operation instead of two, and less visual clutter.
But either works fine, of course---the important point is to decay the array
into a pointer.

Will change it, on the theory that your reaction is typical.

+
+  // Construct an ArrayView for any type U that has a size() method whose
+  // return value converts implicitly to size_t, and a data() method whose
+  // return value converts implicitly to T*. In particular, this means we allow
+  // conversion from ArrayView<T> to ArrayView<const T>, but not the other way
+  // around. Other allowed conversions include std::vector<T> to ArrayView<T>
+  // and rtc::Buffer to ArrayView<uint8_t>.
+  template <typename U>
+  ArrayView(U& u) : ArrayView(u.data(), u.size()) {}
+  // TODO(kwiberg): Remove the special case for std::vector (and the include of
+  // <vector>); it is handled by the general case in C++11, where std::vector
+  // has a data() method.
+  template <typename U>
+  ArrayView(std::vector<U>& u)
+      : ArrayView(u.empty() ? nullptr : &u[0], u.size()) {}
+
+  // Indexing, size, and iteration. These allow mutation even if the ArrayView
+  // is const, because the ArrayView doesn't own the array. (To prevent
+  // mutation, use ArrayView<const T>.)
+  size_t size() const { return size_; }
+  T* data() const { return data_; }

[the sun, 2015/10/21 14:06:34]

RTC_DCHECK(data_ != nullptr)
here and below
+ death tests

[kwiberg-webrtc, 2015/10/22 11:35:33]

We already check in the constructor that data_ is null iff size_ is zero. Do you
still think we need checks here too?

Will add a death test that verifies that the check in the constructor works.
(While doing this, I ended up setting data_ to nullptr if the user passes size
== 0, because the user might compute the size, and having to special-case zero
in that computation would be nonobvious.)

[the sun, 2015/10/22 12:02:49]

I think we still need checks, at the very least in the iterators, since they'd
typically be used in a range-based loop.

Logically, there's no need for the null check in operator[], but I think there
is no harm in adding one to make the conditions explicit and harder for a
maintainer to mess up.

You could argue that data() should be able to return the null and leave it to
the client to check (who knows, maybe an ArrayView could be default c-ted, then
assigned to?), but I would have erred on the side of caution in this case.

[kwiberg-webrtc, 2015/10/22 13:07:00]

Checks in the iterators would take some work. Currently we use plain
pointers for iterators, but a checking variant would use a small class
with e.g. pointer to the ArrayView + current index; we could then
ensure that no one creates an out-of-bounds iterator, dereferences the
end iterator, etc.

Would you like me to build that? In a follow-up CL?
OK. I'll add it. Paranoid Edition on the way...
It was already the case that data() returned null iff size was
zero. The Paranoid Edition makes triple sure.
That's the case, yes. I even exercise it in the tests.
Behold the Paranoid Edition!

+  T& operator[](size_t idx) const {
+    RTC_DCHECK_LT(idx, size_);
+    return data_[idx];
+  }
+  T* begin() const { return data_; }
+  T* end() const { return data_ + size_; }
+  const T* cbegin() const { return data_; }
+  const T* cend() const { return data_ + size_; }
+
+ private:
+  // Invariant: !data_ iff size_ == 0.
+  T* data_;
+  size_t size_;
+};
+
+}  // namespace rtc
+
+#endif  // WEBRTC_BASE_ARRAY_VIEW_H_