ArrayView: Support compile-time constant sizes

ArrayView: Support compile-time constant sizes

BUG=none

Review-Url: https://codereview.webrtc.org/2667383006
Cr-Commit-Position: refs/heads/master@{#16981}
Committed: https://chromium.googlesource.com/external/webrtc/+/bfc7f02d79e217575ae8cceb43d11f0d7d3e6606

[kwiberg-webrtc, 2017-02-02 22:56:41]

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

[commit-bot: I haz the power, 2017-02-02 22:56:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-02-02 23:03:35]

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

[commit-bot: I haz the power, 2017-02-02 23:03:36]

Dry run: Try jobs failed on following builders:
  win_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/22779)
  win_x64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/22260)

[kwiberg-webrtc, 2017-02-06 03:29:14]

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

[commit-bot: I haz the power, 2017-02-06 03:29:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-02-06 03:36:22]

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

[commit-bot: I haz the power, 2017-02-06 03:36:23]

Dry run: Try jobs failed on following builders:
  win_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/22849)

[kwiberg-webrtc, 2017-02-06 04:10:52]

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

[commit-bot: I haz the power, 2017-02-06 04:11:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-02-06 04:16:30]

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

[commit-bot: I haz the power, 2017-02-06 04:16:30]

Dry run: Try jobs failed on following builders:
  win_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/22851)
  win_x64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/5933)

[kwiberg-webrtc, 2017-02-24 01:38:57]

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

[commit-bot: I haz the power, 2017-02-24 01:38:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[kwiberg-webrtc, 2017-02-24 02:33:08]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-02-24 02:33:15]

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

[commit-bot: I haz the power, 2017-02-24 02:33:16]

Dry run: This issue passed the CQ dry run.

[kwiberg-webrtc, 2017-02-24 02:33:17]

Patchset #1 (id:20001) has been deleted

[kwiberg-webrtc, 2017-02-24 02:33:28]

Patchset #1 (id:40001) has been deleted

[kwiberg-webrtc, 2017-02-24 02:33:36]

Patchset #1 (id:60001) has been deleted

[kwiberg-webrtc, 2017-02-24 02:48:08]

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

[kwiberg-webrtc, 2017-02-24 02:48:08]

I finally made this compile on Windows... PTAL.

Using ptrdiff_t for the size and taking -4711 to mean "variable size" is perhaps
not pretty, but it's an implementation detail that shouldn't matter to users. It
*may* be possible to instead use an optional<size_t>-like type with constexpr
constructors, but I haven't tried it and am not sure it would be worth the extra
complexity even if it did work.

[ossu, 2017-03-01 23:16:00]

This is pretty cool. I've a few questions about the design, though. Some are
probably due to jetlag. :)
What's the rationale behind ArrayView<T, 0>? Or, more specifically, the
specialization? When would you want a zero-sized array as input?
How do these types convert between each other? Can I have a function that takes,
say, an ArrayView<int, 10> and send it an ArrayView of indeterminate size as
well, and get runtime error checking?

Oh, and what was the trick to getting it to work on Windows?

https://codereview.webrtc.org/2667383006/diff/80001/webrtc/base/array_view.h
File webrtc/base/array_view.h (right):

https://codereview.webrtc.org/2667383006/diff/80001/webrtc/base/array_view.h#...
webrtc/base/array_view.h:184: static_assert(U::size() == Size, "Sizes must match
exactly");
Why can't the converted-to size be <= the converted-from's size?

https://codereview.webrtc.org/2667383006/diff/80001/webrtc/base/array_view.h#...
webrtc/base/array_view.h:217: ArrayView<T> subview(size_t offset, size_t size)
const {
I guess this is the way to convert to a smaller ArrayView, if wanted. Clamping
the size seems off to me. If I want a subview that's 100 entries big, why would
I accept one that's only 3 entries? Perhaps the non-sized variant could scale
automatically, but the sized one would be stricter?

On the other hand, maybe that just means a lot more std:min() expressions
littered throughout the code. Though that's probably less surprising when read.

https://codereview.webrtc.org/2667383006/diff/80001/webrtc/base/array_view_un...
File webrtc/base/array_view_unittest.cc (right):

https://codereview.webrtc.org/2667383006/diff/80001/webrtc/base/array_view_un...
webrtc/base/array_view_unittest.cc:289: EXPECT_FALSE(av.begin());
Wouldn't the "correct" test be to check that av.begin() == av.end() etc. for
these tests with empty ArrayViews? 
Isn't requiring it to be false (i.e. nullptr) too strict?

[kwiberg-webrtc, 2017-03-02 03:00:39]

https://codereview.webrtc.org/2667383006/diff/80001/webrtc/base/array_view.h
File webrtc/base/array_view.h (right):

https://codereview.webrtc.org/2667383006/diff/80001/webrtc/base/array_view.h#...
webrtc/base/array_view.h:184: static_assert(U::size() == Size, "Sizes must match
exactly");
On 2017/03/01 23:16:00, ossu wrote:
> Why can't the converted-to size be <= the converted-from's size?

Well, it *could*, but IMO what you want for fixed-size array thingies is for the
compiler to verify that the sizes match, not for it to go, "Hey, yeah, I guess I
can fit this square peg into this round hole because its sides are at most
1/sqrt(2) times the hole's diameter."

https://codereview.webrtc.org/2667383006/diff/80001/webrtc/base/array_view.h#...
webrtc/base/array_view.h:217: ArrayView<T> subview(size_t offset, size_t size)
const {
On 2017/03/01 23:16:00, ossu wrote:
> I guess this is the way to convert to a smaller ArrayView, if wanted. Clamping
> the size seems off to me. If I want a subview that's 100 entries big, why
would
> I accept one that's only 3 entries? Perhaps the non-sized variant could scale
> automatically, but the sized one would be stricter?
> 
> On the other hand, maybe that just means a lot more std:min() expressions
> littered throughout the code. Though that's probably less surprising when
read.

Yes, it can be argued both ways. But this was the existing behavior, and also
how std::string_view::substr() and the Google-internal ArraySlice do it.

https://codereview.webrtc.org/2667383006/diff/80001/webrtc/base/array_view_un...
File webrtc/base/array_view_unittest.cc (right):

https://codereview.webrtc.org/2667383006/diff/80001/webrtc/base/array_view_un...
webrtc/base/array_view_unittest.cc:289: EXPECT_FALSE(av.begin());
On 2017/03/01 23:16:00, ossu wrote:
> Wouldn't the "correct" test be to check that av.begin() == av.end() etc. for
> these tests with empty ArrayViews? 
> Isn't requiring it to be false (i.e. nullptr) too strict?

You're right. .data() is required to be null for an empty ArrayView, but we
shouldn't require its iterators to implicitly convert to false.

[kwiberg-webrtc, 2017-03-02 03:07:59]

On 2017/03/01 23:16:00, ossu wrote:
> This is pretty cool. I've a few questions about the design, though. Some are
> probably due to jetlag. :)
> What's the rationale behind ArrayView<T, 0>? Or, more specifically, the
> specialization? When would you want a zero-sized array as input?

I don't expect it to come up other than in templates where the size is a
parameter. But if I hadn't made a specialization for size 0, I would've had to
put the corresponding conditionals in the base for general fixed-size
ArrayViews.

> How do these types convert between each other? Can I have a function that
takes,
> say, an ArrayView<int, 10> and send it an ArrayView of indeterminate size as
> well, and get runtime error checking?

No. You get implicit conversions only if the compiler can guarantee that they
work. I think that's what's most useful, since the whole point of the exercise
is to be able to pass fixed-size arrays around without having to DCHECK their
sizes.

> Oh, and what was the trick to getting it to work on Windows?

I reimplemented the same thing in different ways until it worked. :-)
Specifically, this version has had some enable_ifs converted to static_assert in
places where we didn't really need SFINAE.

[kwiberg-webrtc, 2017-03-02 14:32:57]

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

[commit-bot: I haz the power, 2017-03-02 14:33:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[kwiberg-webrtc, 2017-03-02 14:35:59]

New patch set uploaded with the .begin()/.end() changes.

[commit-bot: I haz the power, 2017-03-02 14:58:13]

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

[commit-bot: I haz the power, 2017-03-02 14:58:14]

Dry run: This issue passed the CQ dry run.

[ossu, 2017-03-02 17:54:07]

lgtm % the begin/cbegin.

https://codereview.webrtc.org/2667383006/diff/80001/webrtc/base/array_view.h
File webrtc/base/array_view.h (right):

https://codereview.webrtc.org/2667383006/diff/80001/webrtc/base/array_view.h#...
webrtc/base/array_view.h:184: static_assert(U::size() == Size, "Sizes must match
exactly");
On 2017/03/02 03:00:39, kwiberg-webrtc wrote:
> On 2017/03/01 23:16:00, ossu wrote:
> > Why can't the converted-to size be <= the converted-from's size?
> 
> Well, it *could*, but IMO what you want for fixed-size array thingies is for
the
> compiler to verify that the sizes match, not for it to go, "Hey, yeah, I guess
I
> can fit this square peg into this round hole because its sides are at most
> 1/sqrt(2) times the hole's diameter."

Yeah, maybe having it implicit is too magical. I'm just wondering, if we have
something that takes an ArrayView<int, 10> and in turn wants to call a
sub-function on the two halves, where the sub-function takes an ArrayView<int,
5>, how we'd go about that as easily and securely as possible. (Without manual
pointer manipulation).

Maybe outside of the scope of this CL, and until we actually need it, but a
subview<index, size> call on static-sized ArrayViews would be sweet!

https://codereview.webrtc.org/2667383006/diff/80001/webrtc/base/array_view.h#...
webrtc/base/array_view.h:217: ArrayView<T> subview(size_t offset, size_t size)
const {
On 2017/03/02 03:00:39, kwiberg-webrtc wrote:
> On 2017/03/01 23:16:00, ossu wrote:
> > I guess this is the way to convert to a smaller ArrayView, if wanted.
Clamping
> > the size seems off to me. If I want a subview that's 100 entries big, why
> would
> > I accept one that's only 3 entries? Perhaps the non-sized variant could
scale
> > automatically, but the sized one would be stricter?
> > 
> > On the other hand, maybe that just means a lot more std:min() expressions
> > littered throughout the code. Though that's probably less surprising when
> read.
> 
> Yes, it can be argued both ways. But this was the existing behavior, and also
> how std::string_view::substr() and the Google-internal ArraySlice do it.

Alright. SGTM!

https://codereview.webrtc.org/2667383006/diff/100001/webrtc/base/array_view_u...
File webrtc/base/array_view_unittest.cc (right):

https://codereview.webrtc.org/2667383006/diff/100001/webrtc/base/array_view_u...
webrtc/base/array_view_unittest.cc:290: EXPECT_EQ(av.begin(), av.end());
Shouldn't these be cbegin and cend? Right now you're checking the same thing
twice. :)

[kwiberg-webrtc, 2017-03-02 19:22:41]

https://codereview.webrtc.org/2667383006/diff/80001/webrtc/base/array_view.h
File webrtc/base/array_view.h (right):

https://codereview.webrtc.org/2667383006/diff/80001/webrtc/base/array_view.h#...
webrtc/base/array_view.h:184: static_assert(U::size() == Size, "Sizes must match
exactly");
On 2017/03/02 17:54:07, ossu wrote:
> On 2017/03/02 03:00:39, kwiberg-webrtc wrote:
> > On 2017/03/01 23:16:00, ossu wrote:
> > > Why can't the converted-to size be <= the converted-from's size?
> > 
> > Well, it *could*, but IMO what you want for fixed-size array thingies is for
> the
> > compiler to verify that the sizes match, not for it to go, "Hey, yeah, I
guess
> I
> > can fit this square peg into this round hole because its sides are at most
> > 1/sqrt(2) times the hole's diameter."
> 
> Yeah, maybe having it implicit is too magical. I'm just wondering, if we have
> something that takes an ArrayView<int, 10> and in turn wants to call a
> sub-function on the two halves, where the sub-function takes an ArrayView<int,
> 5>, how we'd go about that as easily and securely as possible. (Without manual
> pointer manipulation).
> 
> Maybe outside of the scope of this CL, and until we actually need it, but a
> subview<index, size> call on static-sized ArrayViews would be sweet!

Yes, a subview() with only template arguments was what I was thinking too.

https://codereview.webrtc.org/2667383006/diff/100001/webrtc/base/array_view_u...
File webrtc/base/array_view_unittest.cc (right):

https://codereview.webrtc.org/2667383006/diff/100001/webrtc/base/array_view_u...
webrtc/base/array_view_unittest.cc:290: EXPECT_EQ(av.begin(), av.end());
On 2017/03/02 17:54:07, ossu wrote:
> Shouldn't these be cbegin and cend? Right now you're checking the same thing
> twice. :)

Done.

[kwiberg-webrtc, 2017-03-02 19:22:47]

The CQ bit was checked by kwiberg@webrtc.org

[kwiberg-webrtc, 2017-03-02 19:22:48]

The patchset sent to the CQ was uploaded after l-g-t-m from ossu@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2667383006/#ps120001 (title:
"Add missing "c"s")

[commit-bot: I haz the power, 2017-03-02 19:22:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-02 20:33:51]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1488482567608160,
"parent_rev": "51ba2e2ed87e4f2422c43c6783ffd1bda4ee4782", "commit_rev":
"bfc7f02d79e217575ae8cceb43d11f0d7d3e6606"}

[commit-bot: I haz the power, 2017-03-02 20:33:54]

Description was changed from

==========
ArrayView: Support compile-time constant sizes

BUG=none
==========

to

==========
ArrayView: Support compile-time constant sizes

BUG=none

Review-Url: https://codereview.webrtc.org/2667383006
Cr-Commit-Position: refs/heads/master@{#16981}
Committed:
https://chromium.googlesource.com/external/webrtc/+/bfc7f02d79e217575ae8cceb4...
==========

[commit-bot: I haz the power, 2017-03-02 20:33:55]

Committed patchset #3 (id:120001) as
https://chromium.googlesource.com/external/webrtc/+/bfc7f02d79e217575ae8cceb4...