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

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

They are invoked with the maximum size of the data to be added, and a
callable that generates that data, like this:

buffer.AppendData(10, [] (rtc::ArrayView<uint8_t> av) {
    for (uint8_t i = 0; i != 5; ++i)
      av[i] = i;

    return 5;
  });

The callable returns the number of bytes actually written, and the
final Buffer size will be adjusted accordingly. SetData and AppendData
both return the number of bytes added (i.e. the return value of the
callable).

These versions will be useful when converting AudioEncoder::Encode to use Buffer rather than raw pointers.

Also added a few tests for the new functionality.

Committed: https://crrev.com/b01c7816a8c4ef34102ead1a3eca4f389e7a8f43
Cr-Commit-Position: refs/heads/master@{#11733}

[ossu, 2016-02-22 11:06:19]

Description was changed from

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

They are invoked with the maximum size of the data to be added, and a
callable that generates that data, like this:

buffer.AppendData(10, [] (rtc::ArrayView<uint8_t> av) {
    for (uint8_t i = 0; i != 5; ++i)
      av[i] = i;

    return 5;
  });

The callable returns the number of bytes actually written, and the
final Buffer size will be adjusted accordingly. SetData and AppendData
both return the number of bytes added (i.e. the return value of the
callable).

Also added a few tests for the new functionality.
==========

to

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

They are invoked with the maximum size of the data to be added, and a
callable that generates that data, like this:

buffer.AppendData(10, [] (rtc::ArrayView<uint8_t> av) {
    for (uint8_t i = 0; i != 5; ++i)
      av[i] = i;

    return 5;
  });

The callable returns the number of bytes actually written, and the
final Buffer size will be adjusted accordingly. SetData and AppendData
both return the number of bytes added (i.e. the return value of the
callable).

These versions will be useful when converting AudioEncoder::Encode to use Buffer
rather than raw pointers.

Also added a few tests for the new functionality.
==========

[ossu, 2016-02-22 11:06:19]

ossu@webrtc.org changed reviewers:
+ kwiberg@webrtc.org, tommi@webrtc.org

[tommi, 2016-02-22 11:24:19]

I'll leave the main review to kwiberg.  Happy to rubber stamp if that's needed.

https://codereview.webrtc.org/1717273002/diff/1/webrtc/base/buffer.h
File webrtc/base/buffer.h (right):

https://codereview.webrtc.org/1717273002/diff/1/webrtc/base/buffer.h#newcode140
webrtc/base/buffer.h:140: assert(IsConsistent());
kwiberg - can we s/assert/RTC_DCHECK in this file?

https://codereview.webrtc.org/1717273002/diff/1/webrtc/base/buffer.h#newcode168
webrtc/base/buffer.h:168: auto base_ptr = data<T>() + old_size;
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*|.

[tommi, 2016-02-22 11:24:28]

The CQ bit was checked by tommi@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-22 11:24:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1717273002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1717273002/1

[commit-bot: I haz the power, 2016-02-22 11:25:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-22 11:25:31]

Dry run: Try jobs failed on following builders:
  ios_arm64_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/7625)
  ios_arm64_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/7543)
  ios_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/12808)

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

https://codereview.webrtc.org/1717273002/diff/1/webrtc/base/buffer.h
File webrtc/base/buffer.h (right):

https://codereview.webrtc.org/1717273002/diff/1/webrtc/base/buffer.h#newcode130
webrtc/base/buffer.h:130: 
The lack of a blank line here was intended to indicate that the comment applied
to all three SetData functions.

https://codereview.webrtc.org/1717273002/diff/1/webrtc/base/buffer.h#newcode136
webrtc/base/buffer.h:136: 
Doc comment here?

https://codereview.webrtc.org/1717273002/diff/1/webrtc/base/buffer.h#newcode140
webrtc/base/buffer.h:140: assert(IsConsistent());
On 2016/02/22 11:24:18, tommi-webrtc wrote:
> kwiberg - can we s/assert/RTC_DCHECK in this file?

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.)

https://codereview.webrtc.org/1717273002/diff/1/webrtc/base/buffer.h#newcode155
webrtc/base/buffer.h:155: 
See previous comment about a blank line (at line 130).

https://codereview.webrtc.org/1717273002/diff/1/webrtc/base/buffer.h#newcode161
webrtc/base/buffer.h:161: 
Doc comment here?

https://codereview.webrtc.org/1717273002/diff/1/webrtc/base/buffer.h#newcode172
webrtc/base/buffer.h:172: RTC_DCHECK_LE(written_bytes, max_bytes);
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.

https://codereview.webrtc.org/1717273002/diff/1/webrtc/base/buffer.h#newcode175
webrtc/base/buffer.h:175: return written_bytes;
Hmm. Why return this rather than, say, void?

https://codereview.webrtc.org/1717273002/diff/1/webrtc/base/buffer_unittest.cc
File webrtc/base/buffer_unittest.cc (right):

https://codereview.webrtc.org/1717273002/diff/1/webrtc/base/buffer_unittest.c...
webrtc/base/buffer_unittest.cc:257: 
Suggestion: also try a mutable lambda (that should work, right?).

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

https://codereview.webrtc.org/1717273002/diff/1/webrtc/base/buffer.h
File webrtc/base/buffer.h (right):

https://codereview.webrtc.org/1717273002/diff/1/webrtc/base/buffer.h#newcode130
webrtc/base/buffer.h:130: 
On 2016/02/22 13:27:26, kwiberg-webrtc wrote:
> The lack of a blank line here was intended to indicate that the comment
applied
> to all three SetData functions.

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.

https://codereview.webrtc.org/1717273002/diff/1/webrtc/base/buffer.h#newcode136
webrtc/base/buffer.h:136: 
On 2016/02/22 13:27:26, kwiberg-webrtc wrote:
> Doc comment here?

Acknowledged.

https://codereview.webrtc.org/1717273002/diff/1/webrtc/base/buffer.h#newcode140
webrtc/base/buffer.h:140: assert(IsConsistent());
On 2016/02/22 13:27:27, kwiberg-webrtc wrote:
> On 2016/02/22 11:24:18, tommi-webrtc wrote:
> > kwiberg - can we s/assert/RTC_DCHECK in this file?
> 
> 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.)

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)

https://codereview.webrtc.org/1717273002/diff/1/webrtc/base/buffer.h#newcode161
webrtc/base/buffer.h:161: 
On 2016/02/22 13:27:27, kwiberg-webrtc wrote:
> Doc comment here?

Acknowledged.

https://codereview.webrtc.org/1717273002/diff/1/webrtc/base/buffer.h#newcode168
webrtc/base/buffer.h:168: auto base_ptr = data<T>() + old_size;
On 2016/02/22 11:24:18, tommi-webrtc wrote:
> 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*|.

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

https://codereview.webrtc.org/1717273002/diff/1/webrtc/base/buffer.h#newcode172
webrtc/base/buffer.h:172: RTC_DCHECK_LE(written_bytes, max_bytes);
On 2016/02/22 13:27:26, kwiberg-webrtc wrote:
> 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.

Acknowledged.

https://codereview.webrtc.org/1717273002/diff/1/webrtc/base/buffer.h#newcode175
webrtc/base/buffer.h:175: return written_bytes;
On 2016/02/22 13:27:27, kwiberg-webrtc wrote:
> Hmm. Why return this rather than, say, void?

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]

https://codereview.webrtc.org/1717273002/diff/1/webrtc/base/buffer.h
File webrtc/base/buffer.h (right):

https://codereview.webrtc.org/1717273002/diff/1/webrtc/base/buffer.h#newcode130
webrtc/base/buffer.h:130: 
On 2016/02/22 14:20:50, ossu wrote:
> On 2016/02/22 13:27:26, kwiberg-webrtc wrote:
> > The lack of a blank line here was intended to indicate that the comment
> applied
> > to all three SetData functions.
> 
> 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.

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

https://codereview.webrtc.org/1717273002/diff/1/webrtc/base/buffer.h#newcode140
webrtc/base/buffer.h:140: assert(IsConsistent());
On 2016/02/22 14:20:50, ossu wrote:
> On 2016/02/22 13:27:27, kwiberg-webrtc wrote:
> > On 2016/02/22 11:24:18, tommi-webrtc wrote:
> > > kwiberg - can we s/assert/RTC_DCHECK in this file?
> > 
> > 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.)
> 
> 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)

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.

https://codereview.webrtc.org/1717273002/diff/1/webrtc/base/buffer.h#newcode175
webrtc/base/buffer.h:175: return written_bytes;
On 2016/02/22 14:20:50, ossu wrote:
> On 2016/02/22 13:27:27, kwiberg-webrtc wrote:
> > Hmm. Why return this rather than, say, void?
> 
> 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.

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.

[ossu, 2016-02-23 09:49:20]

Fixed issues as per comments.

[kwiberg-webrtc, 2016-02-23 10:06:10]

lgtm, but see comments

https://codereview.webrtc.org/1717273002/diff/1/webrtc/base/buffer_unittest.cc
File webrtc/base/buffer_unittest.cc (right):

https://codereview.webrtc.org/1717273002/diff/1/webrtc/base/buffer_unittest.c...
webrtc/base/buffer_unittest.cc:257: 
On 2016/02/22 13:27:27, kwiberg-webrtc wrote:
> Suggestion: also try a mutable lambda (that should work, right?).

Did you want to do this?

https://codereview.webrtc.org/1717273002/diff/20001/webrtc/base/buffer.h
File webrtc/base/buffer.h (right):

https://codereview.webrtc.org/1717273002/diff/20001/webrtc/base/buffer.h#newc...
webrtc/base/buffer.h:147: // insert the data (i.e. starting at the beginning of
the buffer) and should
"insert" -> "write", maybe?

https://codereview.webrtc.org/1717273002/diff/20001/webrtc/base/buffer.h#newc...
webrtc/base/buffer.h:181: // insert the data (i.e. starting at the former end of
the buffer) and should
insert -> write

[ossu, 2016-02-23 10:11:39]

https://codereview.webrtc.org/1717273002/diff/1/webrtc/base/buffer_unittest.cc
File webrtc/base/buffer_unittest.cc (right):

https://codereview.webrtc.org/1717273002/diff/1/webrtc/base/buffer_unittest.c...
webrtc/base/buffer_unittest.cc:257: 
On 2016/02/23 10:06:10, kwiberg-webrtc wrote:
> On 2016/02/22 13:27:27, kwiberg-webrtc wrote:
> > Suggestion: also try a mutable lambda (that should work, right?).
> 
> Did you want to do this?

I'm not sure why I missed this comment. :/
I'll add a test for that - mostly because I've never tried one before. :)

https://codereview.webrtc.org/1717273002/diff/20001/webrtc/base/buffer.h
File webrtc/base/buffer.h (right):

https://codereview.webrtc.org/1717273002/diff/20001/webrtc/base/buffer.h#newc...
webrtc/base/buffer.h:147: // insert the data (i.e. starting at the beginning of
the buffer) and should
On 2016/02/23 10:06:10, kwiberg-webrtc wrote:
> "insert" -> "write", maybe?

Yeah, that's probably clearer. I'll change that.

https://codereview.webrtc.org/1717273002/diff/20001/webrtc/base/buffer.h#newc...
webrtc/base/buffer.h:181: // insert the data (i.e. starting at the former end of
the buffer) and should
On 2016/02/23 10:06:10, kwiberg-webrtc wrote:
> insert -> write

Acknowledged.

[ossu, 2016-02-23 13:08:27]

The CQ bit was checked by ossu@webrtc.org

[ossu, 2016-02-23 13:08:27]

The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1717273002/#ps40001 (title:
"Added a mutable lambda test and fixed two comments.")

[commit-bot: I haz the power, 2016-02-23 13:08:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1717273002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1717273002/40001

[commit-bot: I haz the power, 2016-02-23 13:17:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-23 13:17:47]

Try jobs failed on following builders:
  win_x64_clang_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_rel/build...)

[ossu, 2016-02-23 14:10:52]

The CQ bit was checked by ossu@webrtc.org

[ossu, 2016-02-23 14:10:52]

The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1717273002/#ps60001 (title:
"Fixed missing include of <algorithm> in buffer.cc and bufferqueue.cc")

[commit-bot: I haz the power, 2016-02-23 14:11:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1717273002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1717273002/60001

[commit-bot: I haz the power, 2016-02-23 14:13:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-23 14:13:44]

Try jobs failed on following builders:
  presubmit on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/3625)

[ossu, 2016-02-23 16:58:00]

Alright. Trybots seem happy. Rubber stamp, please! :)

[kwiberg-webrtc, 2016-02-23 17:50:38]

https://codereview.webrtc.org/1717273002/diff/60001/webrtc/base/buffer_unitte...
File webrtc/base/buffer_unittest.cc (right):

https://codereview.webrtc.org/1717273002/diff/60001/webrtc/base/buffer_unitte...
webrtc/base/buffer_unittest.cc:275: EXPECT_NE(buf.data<char>(), nullptr); //
Data is actually stored.
data has a default template argument (uint8_t), so if you want you can say just
.data().

[tommi, 2016-02-23 21:57:33]

Rubberstamp lgtm. 😀

[ossu, 2016-02-24 08:37:24]

The CQ bit was checked by ossu@webrtc.org

[commit-bot: I haz the power, 2016-02-24 08:37:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1717273002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1717273002/60001

[commit-bot: I haz the power, 2016-02-24 09:05:58]

Description was changed from

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

They are invoked with the maximum size of the data to be added, and a
callable that generates that data, like this:

buffer.AppendData(10, [] (rtc::ArrayView<uint8_t> av) {
    for (uint8_t i = 0; i != 5; ++i)
      av[i] = i;

    return 5;
  });

The callable returns the number of bytes actually written, and the
final Buffer size will be adjusted accordingly. SetData and AppendData
both return the number of bytes added (i.e. the return value of the
callable).

These versions will be useful when converting AudioEncoder::Encode to use Buffer
rather than raw pointers.

Also added a few tests for the new functionality.
==========

to

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

They are invoked with the maximum size of the data to be added, and a
callable that generates that data, like this:

buffer.AppendData(10, [] (rtc::ArrayView<uint8_t> av) {
    for (uint8_t i = 0; i != 5; ++i)
      av[i] = i;

    return 5;
  });

The callable returns the number of bytes actually written, and the
final Buffer size will be adjusted accordingly. SetData and AppendData
both return the number of bytes added (i.e. the return value of the
callable).

These versions will be useful when converting AudioEncoder::Encode to use Buffer
rather than raw pointers.

Also added a few tests for the new functionality.
==========

[commit-bot: I haz the power, 2016-02-24 09:05:59]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-02-24 09:06:07]

Description was changed from

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

They are invoked with the maximum size of the data to be added, and a
callable that generates that data, like this:

buffer.AppendData(10, [] (rtc::ArrayView<uint8_t> av) {
    for (uint8_t i = 0; i != 5; ++i)
      av[i] = i;

    return 5;
  });

The callable returns the number of bytes actually written, and the
final Buffer size will be adjusted accordingly. SetData and AppendData
both return the number of bytes added (i.e. the return value of the
callable).

These versions will be useful when converting AudioEncoder::Encode to use Buffer
rather than raw pointers.

Also added a few tests for the new functionality.
==========

to

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

They are invoked with the maximum size of the data to be added, and a
callable that generates that data, like this:

buffer.AppendData(10, [] (rtc::ArrayView<uint8_t> av) {
    for (uint8_t i = 0; i != 5; ++i)
      av[i] = i;

    return 5;
  });

The callable returns the number of bytes actually written, and the
final Buffer size will be adjusted accordingly. SetData and AppendData
both return the number of bytes added (i.e. the return value of the
callable).

These versions will be useful when converting AudioEncoder::Encode to use Buffer
rather than raw pointers.

Also added a few tests for the new functionality.

Committed: https://crrev.com/b01c7816a8c4ef34102ead1a3eca4f389e7a8f43
Cr-Commit-Position: refs/heads/master@{#11733}
==========

[commit-bot: I haz the power, 2016-02-24 09:06:08]

Patchset 4 (id:??) landed as
https://crrev.com/b01c7816a8c4ef34102ead1a3eca4f389e7a8f43
Cr-Commit-Position: refs/heads/master@{#11733}