Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1408403002:
Introduce rtc::ArrayView<T>, which keeps track of an array that it doesn't own (Closed)
|
Created:
5 years, 2 months ago by kwiberg-webrtc Modified:
5 years, 1 month ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionIntroduce rtc::ArrayView<T>, which keeps track of an array that it doesn't own
The main intended use case is as a function argument, replacing the
harder-to-read and harder-to-use separate pointer and size arguments.
It's easier to read because it's just one argument instead of two, and
with clearly defined semantics; it's easier to use because it has
iterators, and will automatically figure out the size of arrays.
BUG=webrtc:5028
R=andrew@webrtc.org, solenberg@webrtc.org
Committed: https://crrev.com/e2a83eee7337e5a8244c7abacc552bf5ef344442
Cr-Commit-Position: refs/heads/master@{#10415}
Patch Set 1 #Patch Set 2 : The last element is end - 1, not end #Patch Set 3 : Avoid implicit size_t -> char conversion #Patch Set 4 : Implicit conversion to ArrayView from many types #Patch Set 5 : Improve tests #
Total comments: 6
Patch Set 6 : Death test for out-of-bounds indexing #Patch Set 7 : Windows compile fix #
Total comments: 12
Patch Set 8 : Review comments + some free extras #Patch Set 9 : Paranoid Edition #
Total comments: 4
Patch Set 10 : Check invariants only in constructor #
Total comments: 2
Patch Set 11 : Nominal singularization of invariant check #Patch Set 12 : Check one more thing in operator[] #Patch Set 13 : rebase #
Messages
Total messages: 40 (7 generated)
Description was changed from ========== Introduce rtc::ArrayView<T>, which keeps track of an array that it doesn't own The main intended use case is as a function argument, replacing the harder-to-read and harder-to-use separate pointer and size arguments. It's easier to read because it's just one argument instead of two, and with clearly defined semantics; it's easier to use because it has iterators, and will automatically figure out the size of arrays. BUG=webrtc:5028 ========== to ========== Introduce rtc::ArrayView<T>, which keeps track of an array that it doesn't own The main intended use case is as a function argument, replacing the harder-to-read and harder-to-use separate pointer and size arguments. It's easier to read because it's just one argument instead of two, and with clearly defined semantics; it's easier to use because it has iterators, and will automatically figure out the size of arrays. BUG=webrtc:5028 ==========
kwiberg@webrtc.org changed reviewers: + andrew@webrtc.org, henrik.lundin@webrtc.org, solenberg@webrtc.org
☘
On 2015/10/19 11:59:01, kwiberg-webrtc wrote: > ☘ Neat! Yes, questions: 1. What are the differences to std::array? How far are we from being able to use the C++11 std library? 2. For the uses I imagine, do we really have to use C-style arrays? Why not std::vector?
On 2015/10/19 12:14:36, the sun wrote: > On 2015/10/19 11:59:01, kwiberg-webrtc wrote: > > ☘ > > Neat! > > Yes, questions: > > 1. What are the differences to std::array? How far are we from being able to use > the C++11 std library? std::array owns---and actually contains---its array. As such, it's an entirely different type, but it would certainly make sense to provide an ArrayView constructor that accepts std::array as an argument. I don't know when we'll be able to use the C++11 standard library, but it doesn't affect this CL, since it has no analog to ArrayView. > 2. For the uses I imagine, do we really have to use C-style arrays? Why not > std::vector? Again, std::vector is not the same sort of thing, since it owns its array. It would make sense to provide an ArrayView constructor that accepts std::vector as an argument. And C-style arrays is just one of the things that can provide the underlying array for an ArrayView. Think of ArrayView<T> as a replacement for a T*, size_t pair when passing array arguments to a function that doesn't care how the underlying array is owned, just that it stays in place for the duration of the call.
On 2015/10/19 12:29:59, kwiberg-webrtc wrote: > On 2015/10/19 12:14:36, the sun wrote: > > On 2015/10/19 11:59:01, kwiberg-webrtc wrote: > > > ☘ > > > > Neat! > > > > Yes, questions: > > > > 1. What are the differences to std::array? How far are we from being able to > use > > the C++11 std library? > > std::array owns---and actually contains---its array. As such, it's an entirely > different type, but it would certainly make sense to provide an ArrayView > constructor that accepts std::array as an argument. > > I don't know when we'll be able to use the C++11 standard library, but it > doesn't affect this CL, since it has no analog to ArrayView. > > > 2. For the uses I imagine, do we really have to use C-style arrays? Why not > > std::vector? > > Again, std::vector is not the same sort of thing, since it owns its array. It > would make sense to provide an ArrayView constructor that accepts std::vector as > an argument. And C-style arrays is just one of the things that can provide the > underlying array for an ArrayView. > > Think of ArrayView<T> as a replacement for a T*, size_t pair when passing array > arguments to a function that doesn't care how the underlying array is owned, > just that it stays in place for the duration of the call. Yes, but I think that's pretty dangerous (without reference counting). Shared ownership without actually sharing the ownership. The point is, those interfaces where we send in array+size, could we instead send in a const vector& or const array&?
On 2015/10/19 11:59:01, kwiberg-webrtc wrote: > ☘ ( ‘-’)人(ﾟ_ﾟ )
On 2015/10/19 12:37:29, the sun wrote: > On 2015/10/19 12:29:59, kwiberg-webrtc wrote: > > On 2015/10/19 12:14:36, the sun wrote: > > > On 2015/10/19 11:59:01, kwiberg-webrtc wrote: > > > > ☘ > > > > > > Neat! > > > > > > Yes, questions: > > > > > > 1. What are the differences to std::array? How far are we from being able to > > use > > > the C++11 std library? > > > > std::array owns---and actually contains---its array. As such, it's an entirely > > different type, but it would certainly make sense to provide an ArrayView > > constructor that accepts std::array as an argument. > > > > I don't know when we'll be able to use the C++11 standard library, but it > > doesn't affect this CL, since it has no analog to ArrayView. > > > > > 2. For the uses I imagine, do we really have to use C-style arrays? Why not > > > std::vector? > > > > Again, std::vector is not the same sort of thing, since it owns its array. It > > would make sense to provide an ArrayView constructor that accepts std::vector > as > > an argument. And C-style arrays is just one of the things that can provide the > > underlying array for an ArrayView. > > > > Think of ArrayView<T> as a replacement for a T*, size_t pair when passing > array > > arguments to a function that doesn't care how the underlying array is owned, > > just that it stays in place for the duration of the call. > > Yes, but I think that's pretty dangerous (without reference counting). Shared > ownership without actually sharing the ownership. No, no shared ownership. The caller remains the sole owner. (Yes, this leaves ArrayView open to the exact same abuse problems that T*, size_t have, in that callees can save a pointer to the array even though they're not supposed to. But this is C++; you can't protect against code that break the rules.) > The point is, those interfaces where we send in array+size, could we instead > send in a const vector& or const array&? You can pass an ArrayView<const T> if you don't want the caller to write to the elements. Is that what you meant?
On 2015/10/19 12:44:34, kwiberg-webrtc wrote: > On 2015/10/19 12:37:29, the sun wrote: > > On 2015/10/19 12:29:59, kwiberg-webrtc wrote: > > > On 2015/10/19 12:14:36, the sun wrote: > > > > On 2015/10/19 11:59:01, kwiberg-webrtc wrote: > > > > > ☘ > > > > > > > > Neat! > > > > > > > > Yes, questions: > > > > > > > > 1. What are the differences to std::array? How far are we from being able > to > > > use > > > > the C++11 std library? > > > > > > std::array owns---and actually contains---its array. As such, it's an > entirely > > > different type, but it would certainly make sense to provide an ArrayView > > > constructor that accepts std::array as an argument. > > > > > > I don't know when we'll be able to use the C++11 standard library, but it > > > doesn't affect this CL, since it has no analog to ArrayView. > > > > > > > 2. For the uses I imagine, do we really have to use C-style arrays? Why > not > > > > std::vector? > > > > > > Again, std::vector is not the same sort of thing, since it owns its array. > It > > > would make sense to provide an ArrayView constructor that accepts > std::vector > > as > > > an argument. And C-style arrays is just one of the things that can provide > the > > > underlying array for an ArrayView. > > > > > > Think of ArrayView<T> as a replacement for a T*, size_t pair when passing > > array > > > arguments to a function that doesn't care how the underlying array is owned, > > > just that it stays in place for the duration of the call. > > > > Yes, but I think that's pretty dangerous (without reference counting). Shared > > ownership without actually sharing the ownership. > > No, no shared ownership. The caller remains the sole owner. (Yes, this leaves > ArrayView open to the exact same abuse problems that T*, size_t have, in that > callees can save a pointer to the array even though they're not supposed to. But > this is C++; you can't protect against code that break the rules.) Indeed, but it's possible to build levees by not using error prone constructs, such as sending around naked pointers. It's possible to make the argument that ArrayView *hides* from the user/reader the fact that is naked pointer, thus giving a sense of false security. > > The point is, those interfaces where we send in array+size, could we instead > > send in a const vector& or const array&? > > You can pass an ArrayView<const T> if you don't want the caller to write to the > elements. Is that what you meant? I'm just wondering how much use there is for this construct, where e.g. a std::vector wouldn't do the job handily?
andrew@webrtc.org changed reviewers: + mgraczyk@chromium.org
+Michael, who was considering importing ArraySlice into webrtc: https://cs.corp.google.com/#piper///depot/google3/util/gtl/array_slice.h We'd talked about replacing some of the audio interfaces which take raw pointers today with ArraySlices instead. Michael, could you comment on whether ArrayView fulfills your requirements? There are some advantages to using the already proven and widely understood (at least inside Google) ArraySlice. At a glance, it looks like the semantics of ArrayView are very similar.
On 2015/10/19 16:41:29, Andrew MacDonald wrote: > +Michael, who was considering importing ArraySlice into webrtc: > https://cs.corp.google.com/#piper///depot/google3/util/gtl/array_slice.h > > We'd talked about replacing some of the audio interfaces which take raw pointers > today with ArraySlices instead. > > Michael, could you comment on whether ArrayView fulfills your requirements? > > There are some advantages to using the already proven and widely understood (at > least inside Google) ArraySlice. At a glance, it looks like the semantics of > ArrayView are very similar. I hadn't seen ArraySlice before, but it's easy to see why they're similar: They're both constructed from the idea of wrapping a (T*, size_t) pair in a nice and safe STL-inspired bundle with support for iterators etc. And a *clearly documented contract that they don't own the data they point to*. Given those design parameters, there isn't a lot of wiggle room.
https://codereview.webrtc.org/1408403002/diff/80001/webrtc/base/array_view.h File webrtc/base/array_view.h (right): https://codereview.webrtc.org/1408403002/diff/80001/webrtc/base/array_view.h#... webrtc/base/array_view.h:60: // mutation, use ArrayView<const T>.) Why not add clearly non-mutating data() and operator[] instead: T* data() { return data_; } T& operator[](size_t idx) { ... const T* data() const { return data_; } const T& operator[](size_t idx) const { ... harder to misuse https://codereview.webrtc.org/1408403002/diff/80001/webrtc/base/array_view_un... File webrtc/base/array_view_unittest.cc (right): https://codereview.webrtc.org/1408403002/diff/80001/webrtc/base/array_view_un... webrtc/base/array_view_unittest.cc:26: ArrayView<char> y = arr; Nice! https://codereview.webrtc.org/1408403002/diff/80001/webrtc/base/array_view_un... webrtc/base/array_view_unittest.cc:150: // EXPECT_EQ('?', z[8]); // DCHECK error (index out of bounds). Use a death test? https://code.google.com/p/googletest/wiki/AdvancedGuide#Death_Tests
https://codereview.webrtc.org/1408403002/diff/80001/webrtc/base/array_view.h File webrtc/base/array_view.h (right): https://codereview.webrtc.org/1408403002/diff/80001/webrtc/base/array_view.h#... webrtc/base/array_view.h:60: // mutation, use ArrayView<const T>.) On 2015/10/21 10:53:42, the sun wrote: > Why not add clearly non-mutating data() and operator[] instead: > > T* data() { return data_; } > T& operator[](size_t idx) { ... > const T* data() const { return data_; } > const T& operator[](size_t idx) const { ... > > harder to misuse We discussed this face to face; the tl;dr is that representing an immutable array with ArrayView<const T> is better than with const ArrayView<T>, since there is no way to prevent users from making a non-const copy of the latter. So I won't change anything. https://codereview.webrtc.org/1408403002/diff/80001/webrtc/base/array_view_un... File webrtc/base/array_view_unittest.cc (right): https://codereview.webrtc.org/1408403002/diff/80001/webrtc/base/array_view_un... webrtc/base/array_view_unittest.cc:150: // EXPECT_EQ('?', z[8]); // DCHECK error (index out of bounds). On 2015/10/21 10:53:42, the sun wrote: > Use a death test? > https://code.google.com/p/googletest/wiki/AdvancedGuide#Death_Tests Yes, I guess that'd work.
All reviewers: I feel pretty much done at this point. Anything still missing? https://codereview.webrtc.org/1408403002/diff/80001/webrtc/base/array_view_un... File webrtc/base/array_view_unittest.cc (right): https://codereview.webrtc.org/1408403002/diff/80001/webrtc/base/array_view_un... webrtc/base/array_view_unittest.cc:150: // EXPECT_EQ('?', z[8]); // DCHECK error (index out of bounds). On 2015/10/21 12:08:55, kwiberg-webrtc wrote: > On 2015/10/21 10:53:42, the sun wrote: > > Use a death test? > > https://code.google.com/p/googletest/wiki/AdvancedGuide#Death_Tests > > Yes, I guess that'd work. Done.
You've got plenty of solid reviewers without me. Removing myself.
henrik.lundin@webrtc.org changed reviewers: - henrik.lundin@webrtc.org
https://codereview.webrtc.org/1408403002/diff/120001/webrtc/base/array_view.h File webrtc/base/array_view.h (right): https://codereview.webrtc.org/1408403002/diff/120001/webrtc/base/array_view.h... webrtc/base/array_view.h:30: ArrayView() : ArrayView(static_cast<T*>(nullptr), 0) {} Is the cast really necessary? https://codereview.webrtc.org/1408403002/diff/120001/webrtc/base/array_view.h... webrtc/base/array_view.h:62: T* data() const { return data_; } RTC_DCHECK(data_ != nullptr) here and below + death tests https://codereview.webrtc.org/1408403002/diff/120001/webrtc/base/array_view_u... File webrtc/base/array_view_unittest.cc (right): https://codereview.webrtc.org/1408403002/diff/120001/webrtc/base/array_view_u... webrtc/base/array_view_unittest.cc:18: #include "webrtc/base/array_view.h" alphabetical order
I am okay with ArrayView not being immutable as long as we remember that the semantics are not the same as ArraySlice. I believe the U -> (u.data(), u.size()) templated constructor should remain implicit so that we can easily share code between webrtc and internal google repos. Undesirable implicit conversions are unlikely to take place because the converted-to type would need data() and size() defined. Types which allow themselves to be implicitly converted to std::vector, etc are unusual and would probably expect this kind of behavior anyway. https://codereview.webrtc.org/1408403002/diff/120001/webrtc/base/array_view.h File webrtc/base/array_view.h (right): https://codereview.webrtc.org/1408403002/diff/120001/webrtc/base/array_view.h... webrtc/base/array_view.h:41: ArrayView(U (&array)[N]) : ArrayView(array + 0, N) {} Why array + 0 instead of &array[0]? https://codereview.webrtc.org/1408403002/diff/120001/webrtc/base/array_view_u... File webrtc/base/array_view_unittest.cc (right): https://codereview.webrtc.org/1408403002/diff/120001/webrtc/base/array_view_u... webrtc/base/array_view_unittest.cc:55: // ArrayView<char> v = z; // Compile error, because can't drop const. Are you going to use EXPECT_DEATH for these commented-out cases? I don't see the point of including these comments here since the unit test doesn't enforce them. They might make sense in documentation but then they should be collected somewhere a user would be more likely to look. Still, EXPECT_DEATH would be better.
lgtm with the other reviewer's comments addressed.
New patch set posted with review comment fixes + some new stuff (tests that call functions, allowing non-null pointer and zero length in constructor call). https://codereview.webrtc.org/1408403002/diff/120001/webrtc/base/array_view.h File webrtc/base/array_view.h (right): https://codereview.webrtc.org/1408403002/diff/120001/webrtc/base/array_view.h... webrtc/base/array_view.h:30: ArrayView() : ArrayView(static_cast<T*>(nullptr), 0) {} On 2015/10/21 14:06:34, the sun wrote: > Is the cast really necessary? 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. https://codereview.webrtc.org/1408403002/diff/120001/webrtc/base/array_view.h... webrtc/base/array_view.h:41: ArrayView(U (&array)[N]) : ArrayView(array + 0, N) {} On 2015/10/22 00:12:26, mgraczyk wrote: > Why array + 0 instead of &array[0]? 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. https://codereview.webrtc.org/1408403002/diff/120001/webrtc/base/array_view.h... webrtc/base/array_view.h:62: T* data() const { return data_; } On 2015/10/21 14:06:34, the sun wrote: > RTC_DCHECK(data_ != nullptr) > here and below > + death tests 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.) https://codereview.webrtc.org/1408403002/diff/120001/webrtc/base/array_view_u... File webrtc/base/array_view_unittest.cc (right): https://codereview.webrtc.org/1408403002/diff/120001/webrtc/base/array_view_u... webrtc/base/array_view_unittest.cc:18: #include "webrtc/base/array_view.h" On 2015/10/21 14:06:34, the sun wrote: > alphabetical order Done. https://codereview.webrtc.org/1408403002/diff/120001/webrtc/base/array_view_u... webrtc/base/array_view_unittest.cc:55: // ArrayView<char> v = z; // Compile error, because can't drop const. On 2015/10/22 00:12:26, mgraczyk wrote: > Are you going to use EXPECT_DEATH for these commented-out cases? Can't. EXPECT_DEATH is for things that kill the test at runtime, such as CHECK or dereferencing a null pointer. But these are compile errors, and we'd need separate infrastructure to test them. Given the effort involved in building that, and the small number of cases where we'd actually use it in WebRTC, I don't see it happening any time soon. > I don't see the point of including these comments here since the > unit test doesn't enforce them. I originally put them here to verify manually that all those templated constructors didn't allow more things through than I intended, then left them in as a sort of documentation. My opinion is that they make sense to keep because they give clear examples of things that aren't supposed to work, but I can be persuaded otherwise. > They might make sense in documentation but then they should be > collected somewhere a user would be more likely to look. The documentation gives a few examples of what is and isn't allowed when converting ArrayView to ArrayView (ArrayView<const T> to ArrayView<T> bad, others allowed) and std::vector to ArrayView (same story there). But mostly it relies on common sense, since ArrayView allows precisely those conversions you'd think it would allow. Collecting the trivial examples of what isn't allowed doesn't make much sense; we should either keep them where they are, or remove them.
https://codereview.webrtc.org/1408403002/diff/120001/webrtc/base/array_view.h File webrtc/base/array_view.h (right): https://codereview.webrtc.org/1408403002/diff/120001/webrtc/base/array_view.h... webrtc/base/array_view.h:62: T* data() const { return data_; } On 2015/10/22 11:35:33, kwiberg-webrtc wrote: > On 2015/10/21 14:06:34, the sun wrote: > > RTC_DCHECK(data_ != nullptr) > > here and below > > + death tests > > 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.) 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.
New patch set posted. https://codereview.webrtc.org/1408403002/diff/120001/webrtc/base/array_view.h File webrtc/base/array_view.h (right): https://codereview.webrtc.org/1408403002/diff/120001/webrtc/base/array_view.h... webrtc/base/array_view.h:62: T* data() const { return data_; } On 2015/10/22 12:02:49, the sun wrote: > I think we still need checks, at the very least in the iterators, > since they'd typically be used in a range-based loop. 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? > 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. OK. I'll add it. Paranoid Edition on the way... > You could argue that data() should be able to return the null and > leave it to the client to check It was already the case that data() returned null iff size was zero. The Paranoid Edition makes triple sure. > (who knows, maybe an ArrayView could be default c-ted, then assigned > to?), That's the case, yes. I even exercise it in the tests. > but I would have erred on the side of caution in this case. Behold the Paranoid Edition!
On 2015/10/22 00:12:27, mgraczyk wrote: > I am okay with ArrayView not being immutable as long as we remember that the > semantics are not the same as ArraySlice. Well, ArrayView<const T> is pretty much the same as ArraySlice<T>, and ArrayView<T> is likewise similar to MutableArraySlice<T>. And since you can expect functions that only want to read data to take an ArrayView<const T> parameter, implicit conversions for function arguments should work the same too. > I believe the U -> (u.data(), u.size()) templated constructor should remain > implicit so that we can easily share code between webrtc and internal google > repos. Undesirable implicit conversions are unlikely to take place because the > converted-to type would need data() and size() defined. Sounds good. > Types which allow > themselves to be implicitly converted to std::vector, etc are unusual and would > probably expect this kind of behavior anyway. Hmm. Will the compiler actually perform a sequence of more than one implicit conversion? I seem to remember trying that some time ago and being disappointed that it didn't, but I might be mistaken---or maybe I wasn't holding it right...
https://codereview.webrtc.org/1408403002/diff/160001/webrtc/base/array_view.h File webrtc/base/array_view.h (right): https://codereview.webrtc.org/1408403002/diff/160001/webrtc/base/array_view.h... webrtc/base/array_view.h:89: void CheckInvariants() const { RTC_DCHECK_EQ(!data_, size_ == 0); } Checking the invariant should only be necessary in the constructors. Besides, this doesn't really cover the case I'm paranoid about. It is still possible to default construct an ArrayView and dereference begin() without a DCHECK() triggering.
https://codereview.webrtc.org/1408403002/diff/160001/webrtc/base/array_view.h File webrtc/base/array_view.h (right): https://codereview.webrtc.org/1408403002/diff/160001/webrtc/base/array_view.h... webrtc/base/array_view.h:89: void CheckInvariants() const { RTC_DCHECK_EQ(!data_, size_ == 0); } On 2015/10/22 13:42:23, the sun wrote: > Checking the invariant should only be necessary in the constructors. Yes, that's my view too. I must have misunderstood what you said, then. I'll remove the invariant checks everywhere except in the constructor. > Besides, this doesn't really cover the case I'm paranoid about. It is still > possible to default construct an ArrayView and dereference begin() without a > DCHECK() triggering. Yes. Can't fix that without making an iterator class, since we can't forbid people to call begin() on an empty ArrayView. Do you want it in a separate CL?---it'll be ~20-30 new lines in this file, and probably twice that in the test.
lgtm https://codereview.webrtc.org/1408403002/diff/160001/webrtc/base/array_view.h File webrtc/base/array_view.h (right): https://codereview.webrtc.org/1408403002/diff/160001/webrtc/base/array_view.h... webrtc/base/array_view.h:89: void CheckInvariants() const { RTC_DCHECK_EQ(!data_, size_ == 0); } On 2015/10/22 13:54:53, kwiberg-webrtc wrote: > On 2015/10/22 13:42:23, the sun wrote: > > Checking the invariant should only be necessary in the constructors. > > Yes, that's my view too. I must have misunderstood what you said, then. I'll > remove the invariant checks everywhere except in the constructor. > > > Besides, this doesn't really cover the case I'm paranoid about. It is still > > possible to default construct an ArrayView and dereference begin() without a > > DCHECK() triggering. > > Yes. Can't fix that without making an iterator class, since we can't forbid > people to call begin() on an empty ArrayView. Do you want it in a separate > CL?---it'll be ~20-30 new lines in this file, and probably twice that in the > test. I'm fine skipping the iterator implementation if I can just get a RTC_DCHECK(data_) in operator[] and the begin(/end pairs.
https://codereview.webrtc.org/1408403002/diff/180001/webrtc/base/array_view.h File webrtc/base/array_view.h (right): https://codereview.webrtc.org/1408403002/diff/180001/webrtc/base/array_view.h... webrtc/base/array_view.h:76: void CheckInvariants() const { RTC_DCHECK_EQ(!data_, size_ == 0); } nit: lose the plural "s"
Karl, you might send out a webrtc-eng PSA after this lands. ArrayView and Maybe will be quite broadly useful I think.
Depluralizing patch set posted https://codereview.webrtc.org/1408403002/diff/160001/webrtc/base/array_view.h File webrtc/base/array_view.h (right): https://codereview.webrtc.org/1408403002/diff/160001/webrtc/base/array_view.h... webrtc/base/array_view.h:89: void CheckInvariants() const { RTC_DCHECK_EQ(!data_, size_ == 0); } On 2015/10/22 14:29:49, the sun wrote: > On 2015/10/22 13:54:53, kwiberg-webrtc wrote: > > On 2015/10/22 13:42:23, the sun wrote: > > > Checking the invariant should only be necessary in the constructors. > > > > Yes, that's my view too. I must have misunderstood what you said, then. I'll > > remove the invariant checks everywhere except in the constructor. > > > > > Besides, this doesn't really cover the case I'm paranoid about. It is still > > > possible to default construct an ArrayView and dereference begin() without a > > > DCHECK() triggering. > > > > Yes. Can't fix that without making an iterator class, since we can't forbid > > people to call begin() on an empty ArrayView. Do you want it in a separate > > CL?---it'll be ~20-30 new lines in this file, and probably twice that in the > > test. > > I'm fine skipping the iterator implementation if I can just get a > RTC_DCHECK(data_) in operator[] and the begin(/end pairs. You already have it in operator[]: we check that index < size_; if that check succeeds, size_ must be >= 1. The invariant checked at construction time then guarantees that !!data_. If I added !!data_ checks in begin() and end(), they'd trigger when you tried to iterate over an empty ArrayView. As I said before, no checks there unless we go the extra mile and make an iterator class. https://codereview.webrtc.org/1408403002/diff/180001/webrtc/base/array_view.h File webrtc/base/array_view.h (right): https://codereview.webrtc.org/1408403002/diff/180001/webrtc/base/array_view.h... webrtc/base/array_view.h:76: void CheckInvariants() const { RTC_DCHECK_EQ(!data_, size_ == 0); } On 2015/10/22 14:31:30, the sun wrote: > nit: lose the plural "s" But then I leak the implementation detail that the number of invariants is one...! Done.
On 2015/10/22 20:58:01, kwiberg-webrtc wrote: > Depluralizing patch set posted > > https://codereview.webrtc.org/1408403002/diff/160001/webrtc/base/array_view.h > File webrtc/base/array_view.h (right): > > https://codereview.webrtc.org/1408403002/diff/160001/webrtc/base/array_view.h... > webrtc/base/array_view.h:89: void CheckInvariants() const { > RTC_DCHECK_EQ(!data_, size_ == 0); } > On 2015/10/22 14:29:49, the sun wrote: > > On 2015/10/22 13:54:53, kwiberg-webrtc wrote: > > > On 2015/10/22 13:42:23, the sun wrote: > > > > Checking the invariant should only be necessary in the constructors. > > > > > > Yes, that's my view too. I must have misunderstood what you said, then. I'll > > > remove the invariant checks everywhere except in the constructor. > > > > > > > Besides, this doesn't really cover the case I'm paranoid about. It is > still > > > > possible to default construct an ArrayView and dereference begin() without > a > > > > DCHECK() triggering. > > > > > > Yes. Can't fix that without making an iterator class, since we can't forbid > > > people to call begin() on an empty ArrayView. Do you want it in a separate > > > CL?---it'll be ~20-30 new lines in this file, and probably twice that in the > > > test. > > > > I'm fine skipping the iterator implementation if I can just get a > > RTC_DCHECK(data_) in operator[] and the begin(/end pairs. > > You already have it in operator[]: we check that index < size_; if that check > succeeds, size_ must be >= 1. The invariant checked at construction time then > guarantees that !!data_. I know. Let me quote my previous comment: "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." Code is write-once-or-twice, read many many times. There is a point to making it simple to read and understand. > If I added !!data_ checks in begin() and end(), they'd trigger when you tried to > iterate over an empty ArrayView. As I said before, no checks there unless we go > the extra mile and make an iterator class. Could you skip setting data_ to nullptr when size is 0? > > https://codereview.webrtc.org/1408403002/diff/180001/webrtc/base/array_view.h > File webrtc/base/array_view.h (right): > > https://codereview.webrtc.org/1408403002/diff/180001/webrtc/base/array_view.h... > webrtc/base/array_view.h:76: void CheckInvariants() const { > RTC_DCHECK_EQ(!data_, size_ == 0); } > On 2015/10/22 14:31:30, the sun wrote: > > nit: lose the plural "s" > > But then I leak the implementation detail that the number of invariants is > one...! > > Done. Thanks.
On 2015/10/22 21:16:33, the sun wrote: > On 2015/10/22 20:58:01, kwiberg-webrtc wrote: > > You already have it in operator[]: we check that index < size_; if > > that check succeeds, size_ must be >= 1. The invariant checked at > > construction time then guarantees that !!data_. > > I know. Let me quote my previous comment: "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." Code is write-once-or-twice, read many many > times. There is a point to making it simple to read and understand. The last time you said that, I added calls to the invariant check everywhere. But I'll add just the !!data_ check to just operator[] if that's what you want. > > If I added !!data_ checks in begin() and end(), they'd trigger > > when you tried to iterate over an empty ArrayView. As I said > > before, no checks there unless we go the extra mile and make an > > iterator class. > > Could you skip setting data_ to nullptr when size is 0? Yes, but what good would it do? Dereferencing data_ when size_ == 0 is illegal no matter what the value of data_ is. (Which is precisely why you want me to insert this check.) If !data_, the process at least dies cleanly without running amok first. Also, if data_ is never null, the check you want would never trigger. (And in the case where we allow size_ == 0 with data_ either null or not, you get either crashes on iteration or no check when calling begin() and end(), and can't statically tell which way it'll be. That seems like the worst option.) Checking in the body of begin() and end() simply doesn't make sense, in a very fundamental way. The check has to be in the iterator's operator*. Setting data_ to null is the poor man's version of that. So (assuming that I've convinced you), is the current state satisfactory, or would you like an iterator class?
On 2015/10/22 22:03:45, kwiberg-webrtc wrote: > On 2015/10/22 21:16:33, the sun wrote: > > On 2015/10/22 20:58:01, kwiberg-webrtc wrote: > > > > You already have it in operator[]: we check that index < size_; if > > > that check succeeds, size_ must be >= 1. The invariant checked at > > > construction time then guarantees that !!data_. > > > > I know. Let me quote my previous comment: "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." Code is write-once-or-twice, read many many > > times. There is a point to making it simple to read and understand. > > The last time you said that, I added calls to the invariant check > everywhere. But I'll add just the !!data_ check to just operator[] if > that's what you want. > > > > If I added !!data_ checks in begin() and end(), they'd trigger > > > when you tried to iterate over an empty ArrayView. As I said > > > before, no checks there unless we go the extra mile and make an > > > iterator class. > > > > Could you skip setting data_ to nullptr when size is 0? > > Yes, but what good would it do? Dereferencing data_ when size_ == 0 is > illegal no matter what the value of data_ is. (Which is precisely why > you want me to insert this check.) If !data_, the process at least > dies cleanly without running amok first. > > Also, if data_ is never null, the check you want would never > trigger. (And in the case where we allow size_ == 0 with data_ either > null or not, you get either crashes on iteration or no check when > calling begin() and end(), and can't statically tell which way it'll > be. That seems like the worst option.) > > Checking in the body of begin() and end() simply doesn't make sense, > in a very fundamental way. The check has to be in the iterator's > operator*. Setting data_ to null is the poor man's version of that. > > So (assuming that I've convinced you), is the current state > satisfactory, or would you like an iterator class? LGTM
The CQ bit was checked by kwiberg@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from andrew@webrtc.org Link to the patchset: https://codereview.webrtc.org/1408403002/#ps220001 (title: "Check one more thing in operator[]")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1408403002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1408403002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/1406)
Message was sent while issue was closed.
Committed patchset #13 (id:240001) manually as e2a83eee7337e5a8244c7abacc552bf5ef344442 (presubmit successful).
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/e2a83eee7337e5a8244c7abacc552bf5ef344442 Cr-Commit-Position: refs/heads/master@{#10415} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
