Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1707693002:
Changed the semantics of Buffer::Clear to not alter the capacity (Closed)
DescriptionChanged the semantics of Buffer::Clear to not alter the capacity
Also added a test for Clear to ensure this invariant holds.
With this change, it is easy to empty a Buffer and reuse its storage. Further down the line, code filling data into a Buffer could be written to just append to it, with the caller determining if the Buffer should first be cleared or not.
There is currently only one use of Buffer::Clear (in AudioEncoderCopyRed::Reset()) and it should benefit from the change, by not requiring a reallocation after Reset.
Committed: https://crrev.com/728012e49fc9f0c4ecda92781abbeda7cfc8a03f
Cr-Commit-Position: refs/heads/master@{#11680}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Rebase onto correct branch. #
Dependent Patchsets: Messages
Total messages: 33 (14 generated)
Description was changed from ========== Changed the semantics of Buffer::Clear to not alter the capacity. Also added a test for Clear to ensure this invariant holds. BUG= ========== to ========== Changed the semantics of Buffer::Clear to not alter the capacity. Also added a test for Clear to ensure this invariant holds. There is currently only one use of Buffer::Clear (in AudioEncoderCopyRed::Reset()) and it should benefit from the change, by not requiring a reallocation after Reset. ==========
ossu@webrtc.org changed reviewers: + henrika@webrtc.org, kwiberg@webrtc.org
Description was changed from ========== Changed the semantics of Buffer::Clear to not alter the capacity. Also added a test for Clear to ensure this invariant holds. There is currently only one use of Buffer::Clear (in AudioEncoderCopyRed::Reset()) and it should benefit from the change, by not requiring a reallocation after Reset. ========== to ========== Changed the semantics of Buffer::Clear to not alter the capacity. Also added a test for Clear to ensure this invariant holds. With this change, it is easy to empty a Buffer and reuse its storage. Further down the line, code filling data into a Buffer could be written to just append to it, with the caller determining if the Buffer should first be cleared or not. There is currently only one use of Buffer::Clear (in AudioEncoderCopyRed::Reset()) and it should benefit from the change, by not requiring a reallocation after Reset. ==========
Please take a look.
Description was changed from ========== Changed the semantics of Buffer::Clear to not alter the capacity. Also added a test for Clear to ensure this invariant holds. With this change, it is easy to empty a Buffer and reuse its storage. Further down the line, code filling data into a Buffer could be written to just append to it, with the caller determining if the Buffer should first be cleared or not. There is currently only one use of Buffer::Clear (in AudioEncoderCopyRed::Reset()) and it should benefit from the change, by not requiring a reallocation after Reset. ========== to ========== Changed the semantics of Buffer::Clear to not alter the capacity Also added a test for Clear to ensure this invariant holds. With this change, it is easy to empty a Buffer and reuse its storage. Further down the line, code filling data into a Buffer could be written to just append to it, with the caller determining if the Buffer should first be cleared or not. There is currently only one use of Buffer::Clear (in AudioEncoderCopyRed::Reset()) and it should benefit from the change, by not requiring a reallocation after Reset. ==========
Looks good, but see comment. Also, I've updated the commit message to have the first line all on its own, git style. https://codereview.webrtc.org/1707693002/diff/1/webrtc/base/buffer.h File webrtc/base/buffer.h (right): https://codereview.webrtc.org/1707693002/diff/1/webrtc/base/buffer.h#newcode184 webrtc/base/buffer.h:184: // buffer has been moved from. Please unit test the "Works even if the buffer has been moved from" part too.
https://codereview.webrtc.org/1707693002/diff/1/webrtc/base/buffer.h File webrtc/base/buffer.h (right): https://codereview.webrtc.org/1707693002/diff/1/webrtc/base/buffer.h#newcode183 webrtc/base/buffer.h:183: // Resets the buffer to zero size without altering capacity. Works even if the "has been moved from"; what exactly does that mean?
https://codereview.webrtc.org/1707693002/diff/1/webrtc/base/buffer.h File webrtc/base/buffer.h (right): https://codereview.webrtc.org/1707693002/diff/1/webrtc/base/buffer.h#newcode184 webrtc/base/buffer.h:184: // buffer has been moved from. On 2016/02/18 11:22:32, kwiberg-webrtc wrote: > Please unit test the "Works even if the buffer has been moved from" part too. The TestMoveConstruct and TestMoveAssign tests already cover this.
On 2016/02/18 11:54:38, henrika_webrtc wrote: > https://codereview.webrtc.org/1707693002/diff/1/webrtc/base/buffer.h > File webrtc/base/buffer.h (right): > > https://codereview.webrtc.org/1707693002/diff/1/webrtc/base/buffer.h#newcode183 > webrtc/base/buffer.h:183: // Resets the buffer to zero size without altering > capacity. Works even if the > "has been moved from"; what exactly does that mean? Heh, two comments on a part I didn't introduce. :) It means that moving the contents from one Buffer object to another, using a move constructor or move assignment, doesn't break the invariant; i.e. even if the contents of a buffer have been ripped out, Clear() can still reinstate it as a working, empty buffer.
lgtm https://codereview.webrtc.org/1707693002/diff/1/webrtc/base/buffer.h File webrtc/base/buffer.h (right): https://codereview.webrtc.org/1707693002/diff/1/webrtc/base/buffer.h#newcode183 webrtc/base/buffer.h:183: // Resets the buffer to zero size without altering capacity. Works even if the On 2016/02/18 11:54:38, henrika_webrtc wrote: > "has been moved from"; what exactly does that mean? That the object has been used as the argument of a move constructor. (Move constructors typically leave their argument in some valid but unusable state, where the destructor is the only method that's guaranteed to work. Individual classes can give stronger guarantees, which is what rtc::Buffer does here.) https://codereview.webrtc.org/1707693002/diff/1/webrtc/base/buffer.h#newcode184 webrtc/base/buffer.h:184: // buffer has been moved from. On 2016/02/18 12:00:10, ossu wrote: > On 2016/02/18 11:22:32, kwiberg-webrtc wrote: > > Please unit test the "Works even if the buffer has been moved from" part too. > > The TestMoveConstruct and TestMoveAssign tests already cover this. Acknowledged.
LGTM
https://codereview.webrtc.org/1707693002/diff/1/webrtc/base/buffer.h File webrtc/base/buffer.h (right): https://codereview.webrtc.org/1707693002/diff/1/webrtc/base/buffer.h#newcode183 webrtc/base/buffer.h:183: // Resets the buffer to zero size without altering capacity. Works even if the Fine, would be nice with a more clear (less "lingo-like") comment to explain that. Thanks.
The CQ bit was checked by ossu@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1707693002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1707693002/1
The CQ bit was unchecked by commit-bot@chromium.org
CLs for remote refs other than refs/pending/heads/master must contain NOTRY=true and NOPRESUBMIT=true in order for the CQ to process them
The CQ bit was checked by ossu@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org, henrika@webrtc.org Link to the patchset: https://codereview.webrtc.org/1707693002/#ps20001 (title: "Rebase onto correct branch.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1707693002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1707693002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/11021)
https://codereview.webrtc.org/1707693002/diff/1/webrtc/base/buffer.h File webrtc/base/buffer.h (right): https://codereview.webrtc.org/1707693002/diff/1/webrtc/base/buffer.h#newcode183 webrtc/base/buffer.h:183: // Resets the buffer to zero size without altering capacity. Works even if the On 2016/02/18 12:28:19, henrika_webrtc wrote: > Fine, would be nice with a more clear (less "lingo-like") comment to explain > that. Thanks. Yes, maybe. But I count 4 mentions of that concept in this file alone, so it would have to be a pointer to an explanation elsewhere. However, a short explanation is unlikely to suffice, since someone who doesn't know that values that have been moved from shouldn't be used likely has more to learn about rvalue references than just that one fact. At which point we'll just end up pointing at something like https://www.chromium.org/rvalue-references (or possibly straight at "https://www.chromium.org/rvalue-references#TOC-5.-You-must-not-use-a-variable-after-calling-std::move-on-it."). But do we want to sprinkle our code with pointers to C++ language tutorials? My thinking is that rvalue references and move semantics are well-known enough by now that we can count on people to at least recognize this as a specific language feature, which they know how to look up if necessary.
Did not notice that it was used in so many places. I am fine as is.
The CQ bit was checked by ossu@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1707693002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1707693002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/11023)
The CQ bit was checked by ossu@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1707693002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1707693002/20001
Message was sent while issue was closed.
Description was changed from ========== Changed the semantics of Buffer::Clear to not alter the capacity Also added a test for Clear to ensure this invariant holds. With this change, it is easy to empty a Buffer and reuse its storage. Further down the line, code filling data into a Buffer could be written to just append to it, with the caller determining if the Buffer should first be cleared or not. There is currently only one use of Buffer::Clear (in AudioEncoderCopyRed::Reset()) and it should benefit from the change, by not requiring a reallocation after Reset. ========== to ========== Changed the semantics of Buffer::Clear to not alter the capacity Also added a test for Clear to ensure this invariant holds. With this change, it is easy to empty a Buffer and reuse its storage. Further down the line, code filling data into a Buffer could be written to just append to it, with the caller determining if the Buffer should first be cleared or not. There is currently only one use of Buffer::Clear (in AudioEncoderCopyRed::Reset()) and it should benefit from the change, by not requiring a reallocation after Reset. ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Changed the semantics of Buffer::Clear to not alter the capacity Also added a test for Clear to ensure this invariant holds. With this change, it is easy to empty a Buffer and reuse its storage. Further down the line, code filling data into a Buffer could be written to just append to it, with the caller determining if the Buffer should first be cleared or not. There is currently only one use of Buffer::Clear (in AudioEncoderCopyRed::Reset()) and it should benefit from the change, by not requiring a reallocation after Reset. ========== to ========== Changed the semantics of Buffer::Clear to not alter the capacity Also added a test for Clear to ensure this invariant holds. With this change, it is easy to empty a Buffer and reuse its storage. Further down the line, code filling data into a Buffer could be written to just append to it, with the caller determining if the Buffer should first be cleared or not. There is currently only one use of Buffer::Clear (in AudioEncoderCopyRed::Reset()) and it should benefit from the change, by not requiring a reallocation after Reset. Committed: https://crrev.com/728012e49fc9f0c4ecda92781abbeda7cfc8a03f Cr-Commit-Position: refs/heads/master@{#11680} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/728012e49fc9f0c4ecda92781abbeda7cfc8a03f Cr-Commit-Position: refs/heads/master@{#11680} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
