Define rtc::BufferT, like rtc::Buffer but for any trivial type

Define rtc::BufferT, like rtc::Buffer but for any trivial type

And redefine rtc::Buffer as

  using Buffer = BufferT<uint8_t>;

(In the long run, I'd like to remove the type alias and rename the
template to just rtc::Buffer, but that requires all current users of
Buffer to start saying Buffer<uint8_t> instead, and since Buffer is
used in the API, we can't do that in one step.)

The immediate reason for the new template is that we'd like to use
BufferT<int16_t> in the AudioDecoder interface.

BUG=webrtc:5801
Committed: https://crrev.com/a4ac4786a8ca503ac3cb15dfb0a7f6a62a9bdd52
Cr-Commit-Position: refs/heads/master@{#12564}

[kwiberg-webrtc, 2016-04-28 11:39:11]

Description was changed from

==========
Turn rtc::Buffer into a template that can be used with any POD type

The immediate reason is that we'd like to use it with int16_t in the
AudioDecoder interface.

BUG=webrtc:5801
==========

to

==========
Define rtc::BufferT, like rtc::Buffer but for any POD type

And redefine rtc::Buffer as

  using Buffer = BufferT<uint8_t>;

(In the long run, I'd like to remove the type alias and rename the
template to just rtc::Buffer, but that requires all current users of
Buffer to start saying Buffer<uint8_t> instead, and since Buffer is
used in the API, we can't do that in one step.)

The immediate reason for the new template is that we'd like to use
BufferT<int16_t> in the AudioDecoder interface.

BUG=webrtc:5801
==========

[kwiberg-webrtc, 2016-04-28 12:12:22]

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

[ossu, 2016-04-28 13:36:53]

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

https://codereview.webrtc.org/1929903002/diff/1/webrtc/base/buffer.h#newcode31
webrtc/base/buffer.h:31: struct BufferCompat {
So it is "buffer compatible" if both types are of size 1 (and are integral) or
if they're of the same exact type (sans constness)?

Is there a reason to allow int8/uint8 to coexist but not allow the same for
int16/uint16 etc.?

https://codereview.webrtc.org/1929903002/diff/1/webrtc/base/buffer.h#newcode33
webrtc/base/buffer.h:33: std::is_integral<T>::value && sizeof(T) == 1
I find ternaries with other logic operators quite confusing; I expect this works
as intended, but parenthesizing each of the three parts would make it clearer.

https://codereview.webrtc.org/1929903002/diff/1/webrtc/base/buffer.h#newcode41
webrtc/base/buffer.h:41: // Unlike std::string/vector, does not initialize data
when expanding capacity.
This isn't really part of this CL, but std::vector doesn't initialize data when
expanding _capacity_, only when changing the _size_. I guess the difference here
is we don't initialize data even when changing the size.

https://codereview.webrtc.org/1929903002/diff/1/webrtc/base/buffer.h#newcode46
webrtc/base/buffer.h:46: static_assert(!std::is_volatile<T>::value, "T may not
be volatile");
Why not? I mean, I don't see any reason to actually use it, but if someone were
to? Maybe the type-conversion things would get more involved, not really making
it worth the added complexity?

https://codereview.webrtc.org/1929903002/diff/1/webrtc/base/buffer.h#newcode194
webrtc/base/buffer.h:194: // The AppendData functions adds data to the end of
the buffer. They accept
Not your fault, but it should be "functions add" rather than "functions adds"

[ossu, 2016-04-28 13:46:21]

Also, just throwing it out there:

We _could_ have Buffer instead of BufferT, if we can live with the default type
of this wider-scoped Buffer being uint8_t.
It's a bit of a blemish, though.

[kwiberg-webrtc, 2016-04-28 21:19:01]

New patch set uploaded.

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

https://codereview.webrtc.org/1929903002/diff/1/webrtc/base/buffer.h#newcode31
webrtc/base/buffer.h:31: struct BufferCompat {
On 2016/04/28 13:36:53, ossu wrote:
> So it is "buffer compatible" if both types are of size 1 (and are integral) or
> if they're of the same exact type (sans constness)?
> 
> Is there a reason to allow int8/uint8 to coexist but not allow the same for
> int16/uint16 etc.?

For historical reasons, callers use Buffer with mostly uint8_t but also some
int8_t and char---all just because there isn't a unique "byte" type in C++. When
I created the current incarnation of Buffer, I chose to simply let it
transparently operate on all of them. It would be a good deed if someone were to
clean up all the callers so that Buffer could stop supporting this.

However, for types other than byte-sized integers, there are no existing callers
to appease, so I'm being as strict as possible.

https://codereview.webrtc.org/1929903002/diff/1/webrtc/base/buffer.h#newcode33
webrtc/base/buffer.h:33: std::is_integral<T>::value && sizeof(T) == 1
On 2016/04/28 13:36:53, ossu wrote:
> I find ternaries with other logic operators quite confusing; I expect this
works
> as intended, but parenthesizing each of the three parts would make it clearer.

Will do. As expected, ?: has a VERY low precedence (the same as the assignment
operators, and beaten only by the comma operator; see e.g.
http://en.cppreference.com/w/cpp/language/operator_precedence), but most
programmers have only a small subset of that table memorized, so parentheses
definitely help.

https://codereview.webrtc.org/1929903002/diff/1/webrtc/base/buffer.h#newcode41
webrtc/base/buffer.h:41: // Unlike std::string/vector, does not initialize data
when expanding capacity.
On 2016/04/28 13:36:53, ossu wrote:
> This isn't really part of this CL, but std::vector doesn't initialize data
when
> expanding _capacity_, only when changing the _size_. I guess the difference
here
> is we don't initialize data even when changing the size.

That's right. Will fix.

https://codereview.webrtc.org/1929903002/diff/1/webrtc/base/buffer.h#newcode46
webrtc/base/buffer.h:46: static_assert(!std::is_volatile<T>::value, "T may not
be volatile");
On 2016/04/28 13:36:53, ossu wrote:
> Why not? I mean, I don't see any reason to actually use it, but if someone
were
> to? Maybe the type-conversion things would get more involved, not really
making
> it worth the added complexity?

Actually, POD = trivial + standard layout, and we only need the former. Will
change.

The const restriction is because the whole design of Buffer rests on the data
being mutable. I guess if you could live with not using most of the methods it
could work, but I haven't seen a potential use for such a thing. Until it seems
like worth the effort to support, just use std::vector or rtc::ArrayView or
something.

The volatile restriction was mostly because it felt nontrivial to guarantee that
we supported volatile properly, and because one never needs to use it in
practice. But we're lucky: types with a top-level volatile qualifier are never
trivially copyable, and thus not trivial, so we don't even need to check for
this explicitly. Will remove.

Oooh, and also: I'll add !std::is_volatile<U> to BufferCompat, to weed out
volatile U. I was going to use is_trivially_copyable, since that makes it more
obvious what we want, but it doesn't seem to be supported by the standard
library we use on Linux. :-(

https://codereview.webrtc.org/1929903002/diff/1/webrtc/base/buffer.h#newcode194
webrtc/base/buffer.h:194: // The AppendData functions adds data to the end of
the buffer. They accept
On 2016/04/28 13:36:53, ossu wrote:
> Not your fault, but it should be "functions add" rather than "functions adds"

Done.

[kwiberg-webrtc, 2016-04-28 21:19:25]

Description was changed from

==========
Define rtc::BufferT, like rtc::Buffer but for any POD type

And redefine rtc::Buffer as

  using Buffer = BufferT<uint8_t>;

(In the long run, I'd like to remove the type alias and rename the
template to just rtc::Buffer, but that requires all current users of
Buffer to start saying Buffer<uint8_t> instead, and since Buffer is
used in the API, we can't do that in one step.)

The immediate reason for the new template is that we'd like to use
BufferT<int16_t> in the AudioDecoder interface.

BUG=webrtc:5801
==========

to

==========
Define rtc::BufferT, like rtc::Buffer but for any trivial type

And redefine rtc::Buffer as

  using Buffer = BufferT<uint8_t>;

(In the long run, I'd like to remove the type alias and rename the
template to just rtc::Buffer, but that requires all current users of
Buffer to start saying Buffer<uint8_t> instead, and since Buffer is
used in the API, we can't do that in one step.)

The immediate reason for the new template is that we'd like to use
BufferT<int16_t> in the AudioDecoder interface.

BUG=webrtc:5801
==========

[kwiberg-webrtc, 2016-04-28 21:25:02]

https://codereview.webrtc.org/1929903002/diff/20001/webrtc/media/base/mediach...
File webrtc/media/base/mediachannel.h (left):

https://codereview.webrtc.org/1929903002/diff/20001/webrtc/media/base/mediach...
webrtc/media/base/mediachannel.h:37: class Buffer;
This is the one place where we're not backwards compatible, since Buffer is now
a type alias and not a class. I doubt any WebRTC users were forward declaring
rtc::Buffer, though.

[kwiberg-webrtc, 2016-04-28 21:27:12]

On 2016/04/28 13:46:21, ossu wrote:
> Also, just throwing it out there:
> 
> We _could_ have Buffer instead of BufferT, if we can live with the default
type
> of this wider-scoped Buffer being uint8_t.
> It's a bit of a blemish, though.

Ah, you'd think so, but no! I actually tried that first, and the result is that
you can't say just Buffer, you need to say Buffer<> to get the regular old
Buffer<uint8_t>. Which breaks all existing users of Buffer.

[tommi, 2016-04-28 22:49:09]

lgtm.

cc-ing jbauch as well, fyi.

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

https://codereview.webrtc.org/1929903002/diff/20001/webrtc/base/buffer.h#newc...
webrtc/base/buffer.h:33: static constexpr bool value =
first use of constexpr in webrtc? nice

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

https://codereview.webrtc.org/1929903002/diff/20001/webrtc/base/buffer_unitte...
webrtc/base/buffer_unittest.cc:355: static const char* kObsidian = "obsidian";
nit: static const char kObsidian[] = "obsidian"

https://codereview.webrtc.org/1929903002/diff/20001/webrtc/base/buffer_unitte...
webrtc/base/buffer_unittest.cc:356: buf2[2]->stone = kObsidian;
nit: and while this will continue to work, I guess &kObsidian[0] would then be
the technically correct syntax.

https://codereview.webrtc.org/1929903002/diff/20001/webrtc/media/base/mediach...
File webrtc/media/base/mediachannel.h (left):

https://codereview.webrtc.org/1929903002/diff/20001/webrtc/media/base/mediach...
webrtc/media/base/mediachannel.h:37: class Buffer;
On 2016/04/28 21:25:01, kwiberg-webrtc wrote:
> This is the one place where we're not backwards compatible, since Buffer is
now
> a type alias and not a class. I doubt any WebRTC users were forward declaring
> rtc::Buffer, though.

Acknowledged.

[kwiberg-webrtc, 2016-04-29 00:00:19]

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

https://codereview.webrtc.org/1929903002/diff/20001/webrtc/base/buffer.h#newc...
webrtc/base/buffer.h:33: static constexpr bool value =
On 2016/04/28 22:49:08, tommi-webrtc wrote:
> first use of constexpr in webrtc? nice

No, not by a long shot. Here's the winner:
https://chromium.googlesource.com/external/webrtc/+/ee6e4272a4cb28ee6cf38bc73...

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

https://codereview.webrtc.org/1929903002/diff/20001/webrtc/base/buffer_unitte...
webrtc/base/buffer_unittest.cc:355: static const char* kObsidian = "obsidian";
On 2016/04/28 22:49:08, tommi-webrtc wrote:
> nit: static const char kObsidian[] = "obsidian"

OK, I guess, but why?

https://codereview.webrtc.org/1929903002/diff/20001/webrtc/base/buffer_unitte...
webrtc/base/buffer_unittest.cc:356: buf2[2]->stone = kObsidian;
On 2016/04/28 22:49:08, tommi-webrtc wrote:
> nit: and while this will continue to work, I guess &kObsidian[0] would then be
> the technically correct syntax.

The current version works too, because of array-to-pointer decay.

[tommi, 2016-04-29 07:46:38]

++lgtm

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

https://codereview.webrtc.org/1929903002/diff/20001/webrtc/base/buffer.h#newc...
webrtc/base/buffer.h:33: static constexpr bool value =
On 2016/04/29 00:00:19, kwiberg-webrtc wrote:
> On 2016/04/28 22:49:08, tommi-webrtc wrote:
> > first use of constexpr in webrtc? nice
> 
> No, not by a long shot. Here's the winner:
>
https://chromium.googlesource.com/external/webrtc/+/ee6e4272a4cb28ee6cf38bc73...

w00t!

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

https://codereview.webrtc.org/1929903002/diff/20001/webrtc/base/buffer_unitte...
webrtc/base/buffer_unittest.cc:355: static const char* kObsidian = "obsidian";
On 2016/04/29 00:00:19, kwiberg-webrtc wrote:
> On 2016/04/28 22:49:08, tommi-webrtc wrote:
> > nit: static const char kObsidian[] = "obsidian"
> 
> OK, I guess, but why?

It doesn't matter much here since it's a unit test. There are couple of reasons
+ consistency.
1)  The space allocated for |*kObsidian = "obsidian"| is sizeof(char*) +
sizeof("obsidian").  The for the [] case, allocating space for the pointer isn't
needed since the constant is the value (and not a pointer to the value).
2) arraysize(kFoo) returns the size of the array, not the pointer.
3) Because of the above, we try to be consistent with doing it the same way all
over.

Since you don't need or care about the first two in this particular test, I
guess only 3 would apply and that's why just mentioned it as a nit.

https://codereview.webrtc.org/1929903002/diff/20001/webrtc/base/buffer_unitte...
webrtc/base/buffer_unittest.cc:356: buf2[2]->stone = kObsidian;
On 2016/04/29 00:00:19, kwiberg-webrtc wrote:
> On 2016/04/28 22:49:08, tommi-webrtc wrote:
> > nit: and while this will continue to work, I guess &kObsidian[0] would then
be
> > the technically correct syntax.
> 
> The current version works too, because of array-to-pointer decay.

yup, that's what I meant by "will continue to work"

[kwiberg-webrtc, 2016-04-29 07:51:47]

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

https://codereview.webrtc.org/1929903002/diff/20001/webrtc/base/buffer_unitte...
webrtc/base/buffer_unittest.cc:355: static const char* kObsidian = "obsidian";
On 2016/04/29 07:46:38, tommi-webrtc wrote:
> On 2016/04/29 00:00:19, kwiberg-webrtc wrote:
> > On 2016/04/28 22:49:08, tommi-webrtc wrote:
> > > nit: static const char kObsidian[] = "obsidian"
> > 
> > OK, I guess, but why?
> 
> It doesn't matter much here since it's a unit test. There are couple of
reasons
> + consistency.
> 1)  The space allocated for |*kObsidian = "obsidian"| is sizeof(char*) +
> sizeof("obsidian").  The for the [] case, allocating space for the pointer
isn't
> needed since the constant is the value (and not a pointer to the value).
> 2) arraysize(kFoo) returns the size of the array, not the pointer.
> 3) Because of the above, we try to be consistent with doing it the same way
all
> over.
> 
> Since you don't need or care about the first two in this particular test, I
> guess only 3 would apply and that's why just mentioned it as a nit.

OK, those are definitely very good reasons. Thanks.

[ossu, 2016-04-29 08:37:12]

On 2016/04/28 21:27:12, kwiberg-webrtc wrote:
> On 2016/04/28 13:46:21, ossu wrote:
> > Also, just throwing it out there:
> > 
> > We _could_ have Buffer instead of BufferT, if we can live with the default
> type
> > of this wider-scoped Buffer being uint8_t.
> > It's a bit of a blemish, though.
> 
> Ah, you'd think so, but no! I actually tried that first, and the result is
that
> you can't say just Buffer, you need to say Buffer<> to get the regular old
> Buffer<uint8_t>. Which breaks all existing users of Buffer.

Ah, right, I see now. Buffer didn't used to be a template, just its members.
Never mind then.
Also, that would probably cement usage of the default version - this way we can
get away from that.

Concerning signedness: so the goal is to remove the flexibility of Buffer and
have it just work with a single type, as soon as no-one is using it "wrongly"?
Makes sense to me.

[ossu, 2016-04-29 09:09:55]

https://codereview.webrtc.org/1929903002/diff/40001/webrtc/base/buffer.h
File webrtc/base/buffer.h (right):

https://codereview.webrtc.org/1929903002/diff/40001/webrtc/base/buffer.h#newc...
webrtc/base/buffer.h:32: struct BufferCompat {
So if we want to get away from allowing flexibility with signedness for bytes,
maybe that should be mentioned in this header, so as to not implicitly claim
that we want it to be used like that.

https://codereview.webrtc.org/1929903002/diff/40001/webrtc/base/buffer.h#newc...
webrtc/base/buffer.h:145: bool operator==(const BufferT& buf) const {
I was sure I'd asked about this but I can't seem to find that comment:
Shouldn't we be able to compare Buffers easily for types other than integers as
well? Either do a version without memcmp for those cases (using some sort of
traits, perhaps?), or have Buffer mention it only does a binary comparison,
which is how we're using it.

[kwiberg-webrtc, 2016-04-29 10:34:46]

New patch set uploaded.

https://codereview.webrtc.org/1929903002/diff/40001/webrtc/base/buffer.h
File webrtc/base/buffer.h (right):

https://codereview.webrtc.org/1929903002/diff/40001/webrtc/base/buffer.h#newc...
webrtc/base/buffer.h:32: struct BufferCompat {
On 2016/04/29 09:09:55, ossu wrote:
> So if we want to get away from allowing flexibility with signedness for bytes,
> maybe that should be mentioned in this header, so as to not implicitly claim
> that we want it to be used like that.

OK, that makes sense. Will do.

https://codereview.webrtc.org/1929903002/diff/40001/webrtc/base/buffer.h#newc...
webrtc/base/buffer.h:145: bool operator==(const BufferT& buf) const {
On 2016/04/29 09:09:55, ossu wrote:
> I was sure I'd asked about this but I can't seem to find that comment:
> Shouldn't we be able to compare Buffers easily for types other than integers
as
> well? Either do a version without memcmp for those cases (using some sort of
> traits, perhaps?), or have Buffer mention it only does a binary comparison,
> which is how we're using it.

Yes, a version of operator== that uses T's operator== sounds doable. I'll try
it.

[ossu, 2016-04-29 10:51:42]

On 2016/04/29 10:34:46, kwiberg-webrtc wrote:
> New patch set uploaded.
> 
> https://codereview.webrtc.org/1929903002/diff/40001/webrtc/base/buffer.h
> File webrtc/base/buffer.h (right):
> 
>
https://codereview.webrtc.org/1929903002/diff/40001/webrtc/base/buffer.h#newc...
> webrtc/base/buffer.h:32: struct BufferCompat {
> On 2016/04/29 09:09:55, ossu wrote:
> > So if we want to get away from allowing flexibility with signedness for
bytes,
> > maybe that should be mentioned in this header, so as to not implicitly claim
> > that we want it to be used like that.
> 
> OK, that makes sense. Will do.
> 
>
https://codereview.webrtc.org/1929903002/diff/40001/webrtc/base/buffer.h#newc...
> webrtc/base/buffer.h:145: bool operator==(const BufferT& buf) const {
> On 2016/04/29 09:09:55, ossu wrote:
> > I was sure I'd asked about this but I can't seem to find that comment:
> > Shouldn't we be able to compare Buffers easily for types other than integers
> as
> > well? Either do a version without memcmp for those cases (using some sort of
> > traits, perhaps?), or have Buffer mention it only does a binary comparison,
> > which is how we're using it.
> 
> Yes, a version of operator== that uses T's operator== sounds doable. I'll try
> it.

lgtm

[kwiberg-webrtc, 2016-04-29 14:28:51]

The CQ bit was checked by kwiberg@webrtc.org

[kwiberg-webrtc, 2016-04-29 14:28:51]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1929903002/#ps60001 (title:
"review changes")

[commit-bot: I haz the power, 2016-04-29 14:28:54]

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

[commit-bot: I haz the power, 2016-04-29 15:00:26]

Description was changed from

==========
Define rtc::BufferT, like rtc::Buffer but for any trivial type

And redefine rtc::Buffer as

  using Buffer = BufferT<uint8_t>;

(In the long run, I'd like to remove the type alias and rename the
template to just rtc::Buffer, but that requires all current users of
Buffer to start saying Buffer<uint8_t> instead, and since Buffer is
used in the API, we can't do that in one step.)

The immediate reason for the new template is that we'd like to use
BufferT<int16_t> in the AudioDecoder interface.

BUG=webrtc:5801
==========

to

==========
Define rtc::BufferT, like rtc::Buffer but for any trivial type

And redefine rtc::Buffer as

  using Buffer = BufferT<uint8_t>;

(In the long run, I'd like to remove the type alias and rename the
template to just rtc::Buffer, but that requires all current users of
Buffer to start saying Buffer<uint8_t> instead, and since Buffer is
used in the API, we can't do that in one step.)

The immediate reason for the new template is that we'd like to use
BufferT<int16_t> in the AudioDecoder interface.

BUG=webrtc:5801
==========

[commit-bot: I haz the power, 2016-04-29 15:00:27]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-01 22:01:59]

Description was changed from

==========
Define rtc::BufferT, like rtc::Buffer but for any trivial type

And redefine rtc::Buffer as

  using Buffer = BufferT<uint8_t>;

(In the long run, I'd like to remove the type alias and rename the
template to just rtc::Buffer, but that requires all current users of
Buffer to start saying Buffer<uint8_t> instead, and since Buffer is
used in the API, we can't do that in one step.)

The immediate reason for the new template is that we'd like to use
BufferT<int16_t> in the AudioDecoder interface.

BUG=webrtc:5801
==========

to

==========
Define rtc::BufferT, like rtc::Buffer but for any trivial type

And redefine rtc::Buffer as

  using Buffer = BufferT<uint8_t>;

(In the long run, I'd like to remove the type alias and rename the
template to just rtc::Buffer, but that requires all current users of
Buffer to start saying Buffer<uint8_t> instead, and since Buffer is
used in the API, we can't do that in one step.)

The immediate reason for the new template is that we'd like to use
BufferT<int16_t> in the AudioDecoder interface.

BUG=webrtc:5801

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

[commit-bot: I haz the power, 2016-05-01 22:02:00]

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