Added functional variants of Buffer::SetData and Buffer::AppendData.

webrtc/base/buffer.h

 /*
  *  Copyright 2004 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_BUFFER_H_
 #define WEBRTC_BASE_BUFFER_H_
 
-#include <algorithm>  // std::swap (pre-C++11)
 #include <cassert>
 #include <cstring>
 #include <memory>
-#include <utility>  // std::swap (C++11 and later)
+#include <utility>
 
+#include "webrtc/base/array_view.h"
 #include "webrtc/base/constructormagic.h"
 #include "webrtc/base/deprecation.h"
 
 namespace rtc {
 
 namespace internal {
 
 // (Internal; please don't use outside this file.) ByteType<T>::t is int if T
 // is uint8_t, int8_t, or char; otherwise, it's a compilation error. Use like
 // this:
@@ skipping 90 matching lines @@
   bool operator!=(const Buffer& buf) const { return !(*this == buf); }
 
   // Replace the contents of the buffer. Accepts the same types as the
   // constructors.
   template <typename T, typename internal::ByteType<T>::t = 0>
   void SetData(const T* data, size_t size) {
     assert(IsConsistent());
     size_ = 0;
     AppendData(data, size);
   }
+

[kwiberg-webrtc, 2016/02/22 13:27:26]

The lack of a blank line here was intended to indicate that the comment applied
to all three SetData functions.

[ossu, 2016/02/22 14:20:50]

Alright. I'm not certain that it worked, though. At least I didn't get it. :(
Maybe I can re-word it, like: "The SetData functions replace the contents of the
buffer [...]" (and similarly for AppendData)

IMO, there being a blank line (or not) doesn't alter how I'd parse the comment
myself, although not having a blank line makes the code run together and a bit
harder to read.

[kwiberg-webrtc, 2016/02/22 18:23:10]

OK, fair enough. But inset blank lines between all pairs of functions in that
case.

   template <typename T, size_t N, typename internal::ByteType<T>::t = 0>
   void SetData(const T(&array)[N]) {
     SetData(array, N);
   }
   void SetData(const Buffer& buf) { SetData(buf.data(), buf.size()); }
 

[kwiberg-webrtc, 2016/02/22 13:27:26]

Doc comment here?

[ossu, 2016/02/22 14:20:50]

Acknowledged.

+  template <typename T = uint8_t, typename F,
+            typename internal::ByteType<T>::t = 0>
+  size_t SetData(size_t max_bytes, F&& setter) {
+    assert(IsConsistent());

[tommi, 2016/02/22 11:24:18]

kwiberg - can we s/assert/RTC_DCHECK in this file?

[kwiberg-webrtc, 2016/02/22 13:27:27]

I presume so. Not in this CL, though.

Oskar, your call if you want to use RTC_DCHECK instead of assert for the code
you add. Please don't use both, though. (For bonus points, follow up with a CL
that replaces all the calls.)

[ossu, 2016/02/22 14:20:50]

Could IsConsistent() be replaced by (or complemented with) something like:
AssertConsistent() that would use which-ever variant we choose (RTC_DCHECK,
probably) or is there a very good reason to keep the DCHECK macro in the
function at which the problem appears? (Like backtraces or error messages being
more informational)

[kwiberg-webrtc, 2016/02/22 18:23:10]

Yes. :-)

RTC_DCHECK prints an error message that includes the line number. If you
centralize it like you describe, you'll always get the same line number.

+    size_ = 0;
+    return AppendData<T>(max_bytes, std::forward<F>(setter));
+  }
+
   // Append data to the buffer. Accepts the same types as the constructors.
   template <typename T, typename internal::ByteType<T>::t = 0>
   void AppendData(const T* data, size_t size) {
     assert(IsConsistent());
     const size_t new_size = size_ + size;
     EnsureCapacity(new_size);
     std::memcpy(data_.get() + size_, data, size);
     size_ = new_size;
     assert(IsConsistent());
   }
+

[kwiberg-webrtc, 2016/02/22 13:27:27]

See previous comment about a blank line (at line 130).

   template <typename T, size_t N, typename internal::ByteType<T>::t = 0>
   void AppendData(const T(&array)[N]) {
     AppendData(array, N);
   }
   void AppendData(const Buffer& buf) { AppendData(buf.data(), buf.size()); }
 

[kwiberg-webrtc, 2016/02/22 13:27:27]

Doc comment here?

[ossu, 2016/02/22 14:20:50]

Acknowledged.

+  template <typename T = uint8_t, typename F,
+            typename internal::ByteType<T>::t = 0>
+  size_t AppendData(size_t max_bytes, F&& setter) {
+    assert(IsConsistent());
+    const size_t old_size = size_;
+    SetSize(old_size + max_bytes);
+    auto base_ptr = data<T>() + old_size;

[tommi, 2016/02/22 11:24:18]

nit: when using auto, it's still good if you can explicitly declare it the type
is a pointer, reference or an object.  Here it sounds like this could be
|auto*|.

[ossu, 2016/02/22 14:20:50]

Makes sense. Though, looking at it now, T * is shorter and likely clearer. I
think I'll use that instead.

+    size_t written_bytes =
+        setter(rtc::ArrayView<T>(base_ptr, max_bytes));
+
+    RTC_DCHECK_LE(written_bytes, max_bytes);

[kwiberg-webrtc, 2016/02/22 13:27:26]

I'd argue this should be a CHECK instead of a DCHECK, and with a custom message.
Callers can easily trigger this by returning the wrong thing.

Also, please include checks.h explicitly.

[ossu, 2016/02/22 14:20:50]

Acknowledged.

+    size_ = old_size + written_bytes;
+    assert(IsConsistent());
+    return written_bytes;

[kwiberg-webrtc, 2016/02/22 13:27:27]

Hmm. Why return this rather than, say, void?

[ossu, 2016/02/22 14:20:50]

The number of bytes written is returned for purely practical reasons. Every
AudioEncoder returns an EncodedInfo structure that contains (among other things)
the number of encoded bytes, which is what the lambda will return. In the
general case, it makes it easier to get that number out of the lambda, rather
than having to set it through a reference.

[kwiberg-webrtc, 2016/02/22 18:23:10]

Hmm. It's very easy to set via a reference, since you're allowed to use [&]
capture. And using the return value means you have to read the docs, which have
to be longer in order to describe this behavior.

It's a detail, though, so I'll leave it up to you.

+  }
+
   // Sets the size of the buffer. If the new size is smaller than the old, the
   // buffer contents will be kept but truncated; if the new size is greater,
   // the existing contents will be kept and the new space will be
   // uninitialized.
   void SetSize(size_t size) {
     EnsureCapacity(size);
     size_ = size;
   }
 
   // Ensure that the buffer size can be increased to at least capacity without
@@ skipping 58 matching lines @@
   }
 
   size_t size_;
   size_t capacity_;
   std::unique_ptr<uint8_t[]> data_;
 };
 
 }  // namespace rtc
 
 #endif  // WEBRTC_BASE_BUFFER_H_