Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1398473004:
Changed queue implementation to the proposed vector-based solution. (Closed)
|
Created:
5 years, 2 months ago by peah-webrtc Modified:
5 years ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, hlundin-webrtc, aluebs-webrtc, bjornv1, ivoc, terelius Base URL:
https://chromium.googlesource.com/external/webrtc.git@lock_unittest_CL Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionChanged queue implementation to the proposed vector-based solution.
Added unit tests.
BUG=webrtc:5099
TBR=hlundin-webrtc
Patch Set 1 #
Total comments: 135
Patch Set 2 : Changes in response to reviewer comments #
Total comments: 56
Patch Set 3 : Added changes in response to reviewer comments. Added further unit tests and reduce the size of the… #
Total comments: 65
Patch Set 4 : Updated the queue and test implementations #
Total comments: 41
Patch Set 5 : Changes in response to comments, mainly revised comments and naming #
Total comments: 18
Patch Set 6 : Changed verifier implementation, added/changed unittests, made changes in response to reviewer comm… #Patch Set 7 : Changed thread-based unittests to be fully deterministic #
Total comments: 25
Patch Set 8 : Changed code comments. Removed redundant unittest code #
Total comments: 32
Patch Set 9 : Changes to code comments, unittest changes, annotation changes, various minor changes #
Total comments: 32
Patch Set 10 : Added unittest for the Clear function. Did lots of renaming. Extended the test for full queue to en… #
Total comments: 52
Patch Set 11 : Added dchecks, minor code comment updates. #
Total comments: 2
Patch Set 12 : Removed redundant DCHECKs #
Total comments: 2
Patch Set 13 : Unittest updates and other changes in response to comments #
Total comments: 2
Patch Set 14 : Removed and added DCHECKS so that they make sense #Patch Set 15 : Added copy assignment constructor together with unittest. #Patch Set 16 : Made the copy assignment operator thread safe #
Total comments: 8
Patch Set 17 : Removed the copy assignment operator together with related tests #Patch Set 18 : Merge from master #Patch Set 19 : Removed old code for integer verifier and added missing statement delimiter #Patch Set 20 : Latest merged code #Patch Set 21 : Merge from other CLs in the list #Patch Set 22 : Merge from previous CL in CL chain #
Total comments: 6
Patch Set 23 : Comment changes and modified the order of parameters for one of the constructors #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 69 (6 generated)
peah@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, kwiberg@webrtc.org, solenberg@google.com, solenberg@webrtc.org
Changed the queue implementation to the proposed vector-based solution. Added unit tests for the tests.
https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/swapped_nonblocking_queue.h (right): https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:33: explicit SwappedNonBlockingQueue<T>(size_t initial_size, const T& initializer) Remove "explicit". "initial_size" -> "max_size" or just "size"? Size isn't dynamic, right? Add comment on what initializer is. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:36: for (typename std::vector<T>::iterator it = queue_.begin(); for (auto it = ... ? https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:37: it != queue_.end(); ++it) Always use braces, please. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:42: void Reset() { Really needed? https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:49: // The input is replaced by an undefined chunk of type T. replaced by -> swapped with https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:55: size_t next_write_index_ = No trailing underscore on local variables. Can you just size_t next_write_index = (last_write_index_ + 1) % queue.size() ? which you could also put in an internal inline bool CheckIfFull() { ... and use in IsFull() as well. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:75: last_read_index_ = Use modulo instead of conditional https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:82: size_t EmptyQueue() { 1. Is this really needed in the API? should return a bool Should be const. I'd prefer this to be named IsEmpty(). "EmptyQueue" could be an imperative as well. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:88: size_t FullQueue() { 1. Is this really needed in the API? bool, const, IsFull() https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:90: size_t next_write_index_ = Move to internal CheckIfFull() utility https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:100: rtc::CriticalSection crit_queue_; Make mutable
https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/swapped_nonblocking_queue.h (right): https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:21: // This class is to be used as a one-way queue where data is "is to be used as a" -> "is" https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:23: // The read and write operations are protected by a common lock. At this level, it's probably better to just say that it's safe to access this concurrently from multiple threads. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:24: // The queueable objects must all implement a swap() method. That shouldn't be necessary. As long as the usual swapping idiom (http://en.cppreference.com/w/cpp/concept/Swappable) works, we should be able to use T. But you do require T to have a default constructor. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:26: class SwappedNonBlockingQueue { It's a swapping queue, not a swappable one. "NonblockingSwapQueue"? https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:28: explicit SwappedNonBlockingQueue<T>(size_t initial_size) { No need to repeat the <T> here. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:29: queue_.resize(initial_size); std::vector has a constructor that does this. And the size is fixed, right? So drop the "initial". (Or better, use "max_size" as Fredrik suggested below.) https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:30: Reset(); This takes a lock, and just sets two members to zero. I'd say it makes more sense to initialize them without this call. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:39: } std::vector has a constructor for this too. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:40: The constructors are the complicated part of this. Specifically, the problem is that because you have std::vector both in the implementation and as an important potential value of T, you can't use move construction anywhere, and you can't trust the default constructor or even a copy constructor to do the right thing. And you need to give the user control of how those "empty" Ts are constructed. Perhaps a constructor like this would be the least bad option: // TODO(peah): Change to std::vector<T>&& when we have a C++11 // standard library. explicit NonblockingSwapQueue(std::vector<T>* empty_slots) { RTC_CHECK_GE(empty_slots.size(), 1u); using std::swap; swap(queue_, empty_slots); } That way, the caller can prepare the empty slots however she wants (including choosing how many there are), and we just steal them. It's not ideal, since it exposes the fact that the queue uses std::vector internally, but it's probably good enough. For Ts that have a good enough default constructor, you can keep the constructor that takes just a size. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:42: void Reset() { On 2015/10/09 12:09:54, the sun wrote: > Really needed? +1. Also, consider calling it clear() or Clear(); the STL containers call their corresponding methods clear(). https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:50: // Returns false if the queue is full, and true if not. It's not an arbitrary value; it's either one which was created at queue construction, or one which was provided to Remove. This is an important point, because it allows callers to pass in Ts that e.g. own large enough allocations and be sure that Ts they get out here also have that property. It might be a good idea to document how the queue handles "empty" and "full" Ts, and that the constructors take or create "empty" Ts, Insert takes a "full" T and gives back an "empty" one, and Remove takes an "empty" T and gives back a "full" one. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:51: bool Insert(T* const input) { Better drop the const here, or people will confuse it with const T*. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:58: return false; Hmm. This means you'll keep at most size() - 1 elements in the queue. I guess that's unavoidable if you just keep two indices in the range [0..size()-1], since the number of elements need to be in the range [0..size()]. Perhaps it's better to have e.g. next_read_pos_ and num_elements_ as members? https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:61: input->swap(queue_[last_write_index_]); See http://en.cppreference.com/w/cpp/concept/Swappable for the recommended way to swap objects of an arbitrary type. In brief, you should do #include <utility> ... using std::swap; swap(x, y); That will work if T has added a specialization to std::swap (or if the standard library has one built in), if there is a method T::swap(T&), or if there is a non-member swap(T&, T&). https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:75: last_read_index_ = On 2015/10/09 12:09:55, the sun wrote: > Use modulo instead of conditional I'd also have used a conditional, because mod can be expensive. (I'm hoping the compiler would use a conditional move or something instead of a conditional branch.) ... So I tried this. For this program: std::size_t f(std::size_t i, std::size_t s) { return i + 1 < s ? i + 1 : 0; } clang produced this code: 0: 48 ff c7 inc %rdi 3: 31 c0 xor %eax,%eax 5: 48 39 f7 cmp %rsi,%rdi 8: 48 0f 42 c7 cmovb %rdi,%rax c: c3 retq (gcc produced essentially the same code.) We can't expect it to do as well for the mod operation, since that would mean it'd have to keep track of the global invariant that the index always starts out in the range [0..size-1]. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:82: size_t EmptyQueue() { On 2015/10/09 12:09:55, the sun wrote: > 1. Is this really needed in the API? > > should return a bool > > Should be const. > > I'd prefer this to be named IsEmpty(). "EmptyQueue" could be an imperative as > well. > +1 to what Fredrik said. The best thing would be if we didn't need it at all. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:88: size_t FullQueue() { On 2015/10/09 12:09:54, the sun wrote: > 1. Is this really needed in the API? > > bool, const, IsFull() +1 https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:97: std::vector<T> queue_; GUARDED_BY? https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:100: rtc::CriticalSection crit_queue_; On 2015/10/09 12:09:54, the sun wrote: > Make mutable This class is unusable if it's const, so I don't see the point. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc (right): https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc:2: * Copyright (c) 2014 The WebRTC project authors. All Rights Reserved. You do seem to like multiples of 53... https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc:37: explicit Message(const Message& m) { Copy constructor shouldn't be explicit. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc:58: m.info_ = tmp; std::swap(id_, m.id_); std::swap(info_, m.info_); https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc:453: } // namespace webrtc It might just be me, but I think testing of the queue should focus less on thread safety (which is almost impossible to test comprehensively anyway) since that's best established by code review and the annotations you've put in, especially for a piece of code this small and simple. And more on correctness issues such as verifying that Insert fails iff the queue is already full, corner cases such as size == 1, checking that the queue will never do anything to the Ts except swap them, and so on.
https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/swapped_nonblocking_queue.h (right): https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:26: class SwappedNonBlockingQueue { On 2015/10/10 10:16:56, kwiberg-webrtc wrote: > It's a swapping queue, not a swappable one. "NonblockingSwapQueue"? SwapQueue is a great term! Nonblocking or not. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:100: rtc::CriticalSection crit_queue_; On 2015/10/10 10:16:55, kwiberg-webrtc wrote: > On 2015/10/09 12:09:54, the sun wrote: > > Make mutable > > This class is unusable if it's const, so I don't see the point. That's to make it possible for IsEmpty and IsFull to be const declared. OTOH, I'd prefer to remove the lock from IsEmpty/IsFull and instead make them private for now. If we realize there is an actual need for them, we can promote them to be public.
https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/swapped_nonblocking_queue.h (right): https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:100: rtc::CriticalSection crit_queue_; On 2015/10/10 14:00:54, solenberg wrote: > On 2015/10/10 10:16:55, kwiberg-webrtc wrote: > > On 2015/10/09 12:09:54, the sun wrote: > > > Make mutable > > > > This class is unusable if it's const, so I don't see the point. > > That's to make it possible for IsEmpty and IsFull to be const declared. Yeah, OK. No caller is going to make use of that, but it'll catch mistakes in the implementations of those two methods. > OTOH, I'd prefer to remove the lock from IsEmpty/IsFull and instead make them > private for now. If we realize there is an actual need for them, we can promote > them to be public. As I said in an earlier comment, I think it'd be better to use next_read_pos_ and num_elements_ as member variables. One side effect of that is that these two methods will become trivial and can be eliminated if they're only for private use.
Thanks for the great and detailed review!!!! Regarding <It might just be me, but I think testing of the queue should <focus less on <thread safety (which is almost impossible to test <comprehensively anyway) since <that's best established by code review and the annotations <you've put in, <especially for a piece of code this small and simple. And <more on correctness <issues such as verifying that Insert fails iff the queue is <already full, corner <cases such as size == 1, checking that the queue will never <do anything to the <Ts except swap them, and so on. I agree, would it be fine to keep the vector test as it is (but reduce it somewhat), and rewrite the message test to be singlethreaded and to test corner cases and boundary contitions? https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/swapped_nonblocking_queue.h (right): https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:21: // This class is to be used as a one-way queue where data is On 2015/10/10 10:16:56, kwiberg-webrtc wrote: > "is to be used as a" -> "is" Sorry, I don't follow that. Please elaborate further :-) https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:23: // The read and write operations are protected by a common lock. On 2015/10/10 10:16:56, kwiberg-webrtc wrote: > At this level, it's probably better to just say that it's safe to access this > concurrently from multiple threads. Done. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:24: // The queueable objects must all implement a swap() method. On 2015/10/10 10:16:55, kwiberg-webrtc wrote: > That shouldn't be necessary. As long as the usual swapping idiom > (http://en.cppreference.com/w/cpp/concept/Swappable) works, we should be able to > use T. > > But you do require T to have a default constructor. I changed the comment a bit. Could you please verify that it is correct! https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:26: class SwappedNonBlockingQueue { On 2015/10/10 14:00:54, solenberg wrote: > On 2015/10/10 10:16:56, kwiberg-webrtc wrote: > > It's a swapping queue, not a swappable one. "NonblockingSwapQueue"? > > SwapQueue is a great term! Nonblocking or not. Totally agree! I changed it to swap_queue and skipped the nonblocking for now. Ok? https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:28: explicit SwappedNonBlockingQueue<T>(size_t initial_size) { On 2015/10/10 10:16:56, kwiberg-webrtc wrote: > No need to repeat the <T> here. Done. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:29: queue_.resize(initial_size); On 2015/10/10 10:16:56, kwiberg-webrtc wrote: > std::vector has a constructor that does this. And the size is fixed, right? So > drop the "initial". (Or better, use "max_size" as Fredrik suggested below.) Done. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:30: Reset(); On 2015/10/10 10:16:56, kwiberg-webrtc wrote: > This takes a lock, and just sets two members to zero. I'd say it makes more > sense to initialize them without this call. Done. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:33: explicit SwappedNonBlockingQueue<T>(size_t initial_size, const T& initializer) On 2015/10/09 12:09:54, the sun wrote: > Remove "explicit". > > "initial_size" -> "max_size" or just "size"? Size isn't dynamic, right? > > Add comment on what initializer is. Done. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:36: for (typename std::vector<T>::iterator it = queue_.begin(); On 2015/10/09 12:09:55, the sun wrote: > for (auto it = ... ? Done. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:37: it != queue_.end(); ++it) On 2015/10/09 12:09:54, the sun wrote: > Always use braces, please. Done. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:39: } On 2015/10/10 10:16:55, kwiberg-webrtc wrote: > std::vector has a constructor for this too. I removed this one and instead added the one below. Is that ok? https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:40: On 2015/10/10 10:16:56, kwiberg-webrtc wrote: > The constructors are the complicated part of this. Specifically, the problem is > that because you have std::vector both in the implementation and as an important > potential value of T, you can't use move construction anywhere, and you can't > trust the default constructor or even a copy constructor to do the right thing. > And you need to give the user control of how those "empty" Ts are constructed. > > Perhaps a constructor like this would be the least bad option: > > // TODO(peah): Change to std::vector<T>&& when we have a C++11 > // standard library. > explicit NonblockingSwapQueue(std::vector<T>* empty_slots) { > RTC_CHECK_GE(empty_slots.size(), 1u); > using std::swap; > swap(queue_, empty_slots); > } > > That way, the caller can prepare the empty slots however she wants (including > choosing how many there are), and we just steal them. It's not ideal, since it > exposes the fact that the queue uses std::vector internally, but it's probably > good enough. > > For Ts that have a good enough default constructor, you can keep the constructor > that takes just a size. Sounds good! I took the implementation of this one as you suggested. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:40: On 2015/10/10 10:16:56, kwiberg-webrtc wrote: > The constructors are the complicated part of this. Specifically, the problem is > that because you have std::vector both in the implementation and as an important > potential value of T, you can't use move construction anywhere, and you can't > trust the default constructor or even a copy constructor to do the right thing. > And you need to give the user control of how those "empty" Ts are constructed. > > Perhaps a constructor like this would be the least bad option: > > // TODO(peah): Change to std::vector<T>&& when we have a C++11 > // standard library. > explicit NonblockingSwapQueue(std::vector<T>* empty_slots) { > RTC_CHECK_GE(empty_slots.size(), 1u); > using std::swap; > swap(queue_, empty_slots); > } > > That way, the caller can prepare the empty slots however she wants (including > choosing how many there are), and we just steal them. It's not ideal, since it > exposes the fact that the queue uses std::vector internally, but it's probably > good enough. > > For Ts that have a good enough default constructor, you can keep the constructor > that takes just a size. Done. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:42: void Reset() { On 2015/10/10 10:16:56, kwiberg-webrtc wrote: > On 2015/10/09 12:09:54, the sun wrote: > > Really needed? > > +1. Also, consider calling it clear() or Clear(); the STL containers call their > corresponding methods clear(). I think it is needed as it provides lower cost resetting of the state. But I changed it to the more appropriate name Clear(). https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:49: // The input is replaced by an undefined chunk of type T. On 2015/10/09 12:09:54, the sun wrote: > replaced by -> swapped with Done. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:50: // Returns false if the queue is full, and true if not. On 2015/10/10 10:16:55, kwiberg-webrtc wrote: > It's not an arbitrary value; it's either one which was created at queue > construction, or one which was provided to Remove. This is an important point, > because it allows callers to pass in Ts that e.g. own large enough allocations > and be sure that Ts they get out here also have that property. > > It might be a good idea to document how the queue handles "empty" and "full" Ts, > and that the constructors take or create "empty" Ts, Insert takes a "full" T and > gives back an "empty" one, and Remove takes an "empty" T and gives back a "full" > one. Makes sense! I revised the comments of the class and this method accordingly. How do you think it turned out? https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:50: // Returns false if the queue is full, and true if not. On 2015/10/10 10:16:55, kwiberg-webrtc wrote: > It's not an arbitrary value; it's either one which was created at queue > construction, or one which was provided to Remove. This is an important point, > because it allows callers to pass in Ts that e.g. own large enough allocations > and be sure that Ts they get out here also have that property. > > It might be a good idea to document how the queue handles "empty" and "full" Ts, > and that the constructors take or create "empty" Ts, Insert takes a "full" T and > gives back an "empty" one, and Remove takes an "empty" T and gives back a "full" > one. I added some documentation. Please let me know if I should add more! https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:51: bool Insert(T* const input) { On 2015/10/10 10:16:56, kwiberg-webrtc wrote: > Better drop the const here, or people will confuse it with const T*. Done. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:51: bool Insert(T* const input) { On 2015/10/10 10:16:56, kwiberg-webrtc wrote: > Better drop the const here, or people will confuse it with const T*. Done. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:55: size_t next_write_index_ = On 2015/10/09 12:09:54, the sun wrote: > No trailing underscore on local variables. > > Can you just > > size_t next_write_index = (last_write_index_ + 1) % queue.size() ? > > which you could also put in an internal > > inline bool CheckIfFull() { > ... > > and use in IsFull() as well. Did this, but a bit differently. I also kept the short-circuit instead of the modulus operation. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:55: size_t next_write_index_ = On 2015/10/09 12:09:54, the sun wrote: > No trailing underscore on local variables. > > Can you just > > size_t next_write_index = (last_write_index_ + 1) % queue.size() ? > > which you could also put in an internal > > inline bool CheckIfFull() { > ... > > and use in IsFull() as well. I removed those functions because with the current counter for number of elements they become trivial. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:58: return false; On 2015/10/10 10:16:56, kwiberg-webrtc wrote: > Hmm. This means you'll keep at most size() - 1 elements in the queue. I guess > that's unavoidable if you just keep two indices in the range [0..size()-1], > since the number of elements need to be in the range [0..size()]. > > Perhaps it's better to have e.g. next_read_pos_ and num_elements_ as members? Good point. I added the num_elements_ idea, but kept the preexisting last_write_index_ and last_read_index_ even though that 3-index/counter scheme is redundant (only using the two variables next_read_pos_ and num_elements_ you proposed is sufficient). The reason for this is that the index updating becomes simpler, cleaner and cheaper with the redundant scheme. Please let me know what you think! https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:58: return false; On 2015/10/10 10:16:56, kwiberg-webrtc wrote: > Hmm. This means you'll keep at most size() - 1 elements in the queue. I guess > that's unavoidable if you just keep two indices in the range [0..size()-1], > since the number of elements need to be in the range [0..size()]. > > Perhaps it's better to have e.g. next_read_pos_ and num_elements_ as members? I added the num_elements_ as member which simplifies the code a lot and allows for full queue usage. I kept the the two write and read indices, however, even thought they are redundant, as they are slightly less computationally complex to use and makes the code easier to read. Please let me know if you still prefer to have them replaced by only using the next_read_pos_ variable. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:61: input->swap(queue_[last_write_index_]); On 2015/10/10 10:16:56, kwiberg-webrtc wrote: > See http://en.cppreference.com/w/cpp/concept/Swappable for the recommended way > to swap objects of an arbitrary type. In brief, you should do > > #include <utility> > > ... > > using std::swap; > swap(x, y); > > That will work if T has added a specialization to std::swap (or if the standard > library has one built in), if there is a method T::swap(T&), or if there is a > non-member swap(T&, T&). Thanks!!!! Does this look as what you intended? https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:75: last_read_index_ = On 2015/10/10 10:16:56, kwiberg-webrtc wrote: > On 2015/10/09 12:09:55, the sun wrote: > > Use modulo instead of conditional > > I'd also have used a conditional, because mod can be expensive. (I'm hoping the > compiler would use a conditional move or something instead of a conditional > branch.) > > ... > > So I tried this. For this program: > > std::size_t f(std::size_t i, std::size_t s) { > return i + 1 < s ? i + 1 : 0; > } > > clang produced this code: > > 0: 48 ff c7 inc %rdi > 3: 31 c0 xor %eax,%eax > 5: 48 39 f7 cmp %rsi,%rdi > 8: 48 0f 42 c7 cmovb %rdi,%rax > c: c3 retq > > (gcc produced essentially the same code.) > > We can't expect it to do as well for the mod operation, since that would mean > it'd have to keep track of the global invariant that the index always starts out > in the range [0..size-1]. Great investigation! Cool!!! I'll keep the conditional as it is then. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:82: size_t EmptyQueue() { On 2015/10/10 10:16:56, kwiberg-webrtc wrote: > On 2015/10/09 12:09:55, the sun wrote: > > 1. Is this really needed in the API? > > > > should return a bool > > > > Should be const. > > > > I'd prefer this to be named IsEmpty(). "EmptyQueue" could be an imperative as > > well. > > > > +1 to what Fredrik said. The best thing would be if we didn't need it at all. Agree, I removed the function. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:82: size_t EmptyQueue() { On 2015/10/09 12:09:55, the sun wrote: > 1. Is this really needed in the API? > > should return a bool > > Should be const. > > I'd prefer this to be named IsEmpty(). "EmptyQueue" could be an imperative as > well. > Done. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:88: size_t FullQueue() { On 2015/10/10 10:16:56, kwiberg-webrtc wrote: > On 2015/10/09 12:09:54, the sun wrote: > > 1. Is this really needed in the API? > > > > bool, const, IsFull() > > +1 Agree, I removed the function. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:88: size_t FullQueue() { On 2015/10/10 10:16:56, kwiberg-webrtc wrote: > On 2015/10/09 12:09:54, the sun wrote: > > 1. Is this really needed in the API? > > > > bool, const, IsFull() > > +1 Done. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:90: size_t next_write_index_ = On 2015/10/09 12:09:54, the sun wrote: > Move to internal CheckIfFull() utility I removed that functionality, as it with the current element is trivial. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:97: std::vector<T> queue_; On 2015/10/10 10:16:56, kwiberg-webrtc wrote: > GUARDED_BY? Done. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:100: rtc::CriticalSection crit_queue_; On 2015/10/10 17:48:40, kwiberg-webrtc wrote: > On 2015/10/10 14:00:54, solenberg wrote: > > On 2015/10/10 10:16:55, kwiberg-webrtc wrote: > > > On 2015/10/09 12:09:54, the sun wrote: > > > > Make mutable > > > > > > This class is unusable if it's const, so I don't see the point. > > > > That's to make it possible for IsEmpty and IsFull to be const declared. > > Yeah, OK. No caller is going to make use of that, but it'll catch mistakes in > the implementations of those two methods. > > > OTOH, I'd prefer to remove the lock from IsEmpty/IsFull and instead make them > > private for now. If we realize there is an actual need for them, we can > promote > > them to be public. > > As I said in an earlier comment, I think it'd be better to use next_read_pos_ > and num_elements_ as member variables. One side effect of that is that these two > methods will become trivial and can be eliminated if they're only for private > use. Agree to both of you! I removed the IsEmpty and IsFull functions. And did not add mutable for the critical section https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc (right): https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc:2: * Copyright (c) 2014 The WebRTC project authors. All Rights Reserved. On 2015/10/10 10:16:57, kwiberg-webrtc wrote: > You do seem to like multiples of 53... It is a prime number :-) But is it occurring in this file as well? https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc:37: explicit Message(const Message& m) { On 2015/10/10 10:16:57, kwiberg-webrtc wrote: > Copy constructor shouldn't be explicit. Done. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc:58: m.info_ = tmp; On 2015/10/10 10:16:57, kwiberg-webrtc wrote: > std::swap(id_, m.id_); > std::swap(info_, m.info_); Done.
Thanks for the great and detailed review!!!! Regarding <It might just be me, but I think testing of the queue should <focus less on <thread safety (which is almost impossible to test <comprehensively anyway) since <that's best established by code review and the annotations <you've put in, <especially for a piece of code this small and simple. And <more on correctness <issues such as verifying that Insert fails iff the queue is <already full, corner <cases such as size == 1, checking that the queue will never <do anything to the <Ts except swap them, and so on. I agree, would it be fine to keep the vector test as it is (but reduce it somewhat), and rewrite the message test to be singlethreaded and to test corner cases and boundary contitions?
Yes, I think it would be fine to keep the existing test in addition to more traditional unit tests. How long does it take? https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/swapped_nonblocking_queue.h (right): https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:21: // This class is to be used as a one-way queue where data is On 2015/10/12 13:01:06, peah-webrtc wrote: > On 2015/10/10 10:16:56, kwiberg-webrtc wrote: > > "is to be used as a" -> "is" > > Sorry, I don't follow that. Please elaborate further :-) Just replace the left-hand side string with the right-hand side one. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:24: // The queueable objects must all implement a swap() method. On 2015/10/12 13:01:08, peah-webrtc wrote: > On 2015/10/10 10:16:55, kwiberg-webrtc wrote: > > That shouldn't be necessary. As long as the usual swapping idiom > > (http://en.cppreference.com/w/cpp/concept/Swappable) works, we should be able > to > > use T. > > > > But you do require T to have a default constructor. > > I changed the comment a bit. Could you please verify that it is correct! Looks good. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:26: class SwappedNonBlockingQueue { On 2015/10/12 13:01:07, peah-webrtc wrote: > On 2015/10/10 14:00:54, solenberg wrote: > > On 2015/10/10 10:16:56, kwiberg-webrtc wrote: > > > It's a swapping queue, not a swappable one. "NonblockingSwapQueue"? > > > > SwapQueue is a great term! Nonblocking or not. > > Totally agree! I changed it to swap_queue and skipped the nonblocking for now. > Ok? Acknowledged. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:39: } On 2015/10/12 13:01:07, peah-webrtc wrote: > On 2015/10/10 10:16:55, kwiberg-webrtc wrote: > > std::vector has a constructor for this too. > > I removed this one and instead added the one below. Is that ok? No, I count three constructors in the new patch set. See comments there. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:50: // Returns false if the queue is full, and true if not. On 2015/10/12 13:01:07, peah-webrtc wrote: > On 2015/10/10 10:16:55, kwiberg-webrtc wrote: > > It's not an arbitrary value; it's either one which was created at queue > > construction, or one which was provided to Remove. This is an important point, > > because it allows callers to pass in Ts that e.g. own large enough allocations > > and be sure that Ts they get out here also have that property. > > > > It might be a good idea to document how the queue handles "empty" and "full" > Ts, > > and that the constructors take or create "empty" Ts, Insert takes a "full" T > and > > gives back an "empty" one, and Remove takes an "empty" T and gives back a > "full" > > one. > > I added some documentation. Please let me know if I should add more! Acknowledged. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:61: input->swap(queue_[last_write_index_]); On 2015/10/12 13:01:07, peah-webrtc wrote: > On 2015/10/10 10:16:56, kwiberg-webrtc wrote: > > See http://en.cppreference.com/w/cpp/concept/Swappable for the recommended way > > to swap objects of an arbitrary type. In brief, you should do > > > > #include <utility> > > > > ... > > > > using std::swap; > > swap(x, y); > > > > That will work if T has added a specialization to std::swap (or if the > standard > > library has one built in), if there is a method T::swap(T&), or if there is a > > non-member swap(T&, T&). > > Thanks!!!! Does this look as what you intended? Yes. https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:12: #define WEBRTC_MODULES_AUDIO_PROCESSING_SWAPPED_NONBLOCKING_QUEUE_H_ The include guard name needs updating. https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:23: // This class is to be used as a one-way queue where data is It actually *is* a one-way queue. https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:36: class SwapQueue { It might be a good idea to reflect the threadsafeness in the class name, since the usual case is for simple data structures not to be threadsafe. ThreadsafeSwapQueue? https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:38: // Creates a queue of size size. Document that this fills the queue with the specified number of default-constructed Ts. https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:39: explicit SwapQueue(size_t size) { queue_.resize(size); } explicit SwapQueue(size_t size) : queue_(size) {} https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:47: } "explicit" is only needed for one-argument constructors. No <T> needed. Call queue_'s constructor with two arguments: size and initializer. Don't call the other SwapQueue constructor---that will initialize queue_ twice. I don't think the example of T == std::vector in the comment helps. And it's probably unnecessary to have this constructor at all; the first and third one seem sufficient. https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:50: // are formed as in the supplied empty slots input Maybe just // Create a queue with "empty" Ts from the given // vector. (The constructor will steal the contents from // the supplied vector, leaving it empty.) This probably works best in conjunction with a longer explanation of "full" and "empty" Ts, maybe in the class description. https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:56: // is used with an input of vectors having the same size. You repeat this several times. Maybe just once, in the class description? I think it might be useful to phrase it in terms of "full" and "empty" Ts: The Ts passed to (or created by) the queue's constructor are "empty". Insert swaps a "full" T for an "empty" T, and Remove does the opposite. Except for in the constructors and the destructor, the queue never creates or destroys Ts, just swaps them, so it's guaranteed that the "empty" Ts you get from Insert are the same as the "empty" Ts given to Remove (or passed to/created by the constructors). It follows that if you want the "empty" Ts returned by Insert to have some property such as a certain size, just ensure that the Ts you give to the constructor and Remove have that property. Clear is a special case; it moves all Ts currently in the "full" bucket to the "empty" bucket. This makes the queue harder to explain, which is one reason it'd be nice to not have it. Another thing that would make the queue easier to explain would be if you had only the third constructor, so that you could claim simply that it never creates Ts. https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:71: // Inserts a chunk of data into the queue in a thread safe manner. Document thread safety once, in the class description. Every operation should be safe. https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:74: // Returns false if the queue is full, and true if not. Document that the operation is a no-op iff it returns false. https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:98: // Returns false if the queue is empty, and true if not. Same as above: document thread safety once, and say it's a no-op iff it returns false. https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:113: output->swap(queue_[last_read_index_]); Use the same swap idiom as above. https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:123: size_t num_elements_ GUARDED_BY(crit_queue_) = 0; Document the invariants: queue_.size() never changes, 0 <= num_elements_ <= queue_.size(), last_read_index_ + num_elements_ = last_write_index_ mod queue_.size() (with the two indices equal iff the queue is full or empty). And note that by using last_*_index instead of next_*_index, you use the slots in the slightly inelegant order 1, 2, 3, ..., size-1, 0, 1, 2, ...
https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:52: // standard library. Actually, I realized on the way home that we only need the language part of C++11. Like this: explicit SwapQueue(std::vector<T>&& empty_slots) { RTC_CHECK_GE(empty_slots->size(), 1u); using std::swap; swap(queue_, empty_slots); } To create a queue with n default-constructed "empty" slots: SwapQueue<T>(std::vector<T>(n)); With n "empty" slots copy-constructed from t: SwapQueue<T>(std::vector<T>(n, t)); With some more elaborately constructed vector of Ts: std::vector<T> vt; // Fill it! SwapQueue<T>(static_cast<std::vector<T>&&>(vt)); The C++11 standard library would let us use std::move instead of a cast in the third example, but that's all.
Here are some late comments for patch set 1. Please, disregard any that are no longer relevant. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/BUILD.gn (right): https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/BUILD.gn:91: "swapped_nonblocking_queue.h", I wonder if this should be sitting in common_audio instead, together with the RingBuffer and other friends. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/swapped_nonblocking_queue.h (right): https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:23: // The read and write operations are protected by a common lock. On 2015/10/10 10:16:56, kwiberg-webrtc wrote: > At this level, it's probably better to just say that it's safe to access this > concurrently from multiple threads. +1. Spare the implementation details. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:30: Reset(); On 2015/10/10 10:16:56, kwiberg-webrtc wrote: > This takes a lock, and just sets two members to zero. I'd say it makes more > sense to initialize them without this call. OTOH, calling Reset() here makes it more obvious that the Reset method in fact resets the object to the same state it had when constructed. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:37: it != queue_.end(); ++it) On 2015/10/09 12:09:54, the sun wrote: > Always use braces, please. I agree, but I know that braces here would get push-back in Chromium. But, please add them here. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:50: // Returns false if the queue is full, and true if not. Clarify that false if returned if the queue is full already before trying to insert; not if it becomes full by this call. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:67: // Returns false if the queue is empty, and true if not. Again, clarify that false is returned if the queue was already empty, so that no data could be fetched. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc (right): https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc:2: * Copyright (c) 2014 The WebRTC project authors. All Rights Reserved. On 2015/10/10 10:16:57, kwiberg-webrtc wrote: > You do seem to like multiples of 53... I think 53 is just a coincident. 19 is obviously the factor of desire. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc:42: explicit Message(int id, int info) { No need to make a 2-argument constructor explicit, is it? https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc:112: // Thread callback for the reader thread End with . https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc:118: // Thread callback for the writer thread End with . https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc:129: size_t GetNumMessagesRead() { const-declare the method (and make crit_ mutable). https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc:131: return num_messages_read_; I'm not sure, but this seems like the kind of thing that should be able to achieve with an atomic instead of using locks. One thread is making simple increments, and the other is reading the value. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc:146: size_t num_messages_read = GetNumMessagesRead(); const But, you don't need this variable, at least not here. Either move it inside the if-statement below, or simply use the method invocation as index: messages_[GetNumMessagesRead()] https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc:181: SleepMs(rand_r(seed) % (max_sleep + 1)); Will rand_r work across platforms? https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc:186: EXPECT_TRUE(reader_thread_->Start()); ASSERT_TRUE. Makes little sense to continue if thread-starting fails. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc:226: queue_size_ = static_cast<size_t>(testing::get<0>(GetParam())); If you parse these parameters in the constructor instead of in SetUp, you can make all these member variables const. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc:231: // llocate read and write buffers. -> Allocate https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc:268: // Thread callback for the reader thread End with . https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc:274: // Thread callback for the writer thread End with . https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc:309: size_t num_samples_read = GetNumSamplesRead(); const https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc:383: EXPECT_TRUE(reader_thread_->Start()); Again, ASSERT_*? https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc:407: unsigned int reader_seed = 42; Trailing underscore, please. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc:408: unsigned int writer_seed = 37; Trailing underscore, please. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc:419: // Test parameters for the message queue tests. Let the consts live in the anonymous namespace too.
https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:25: // It is safe to access the class concurrently from multiple threads. Will git cl format fix the alignment of comment lines to 80 chars? https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:28: // When T is a vector, one way of us using this class is by leveraging that What you describe is the general intention of this design; it is not limited to vectors. Please describe the general case before talking about a specific case. Also, an example with pseudo code would likely be useful, or referring to a specific unit test case that illustrates clearly the use. https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:36: class SwapQueue { On 2015/10/12 14:05:48, kwiberg-webrtc wrote: > It might be a good idea to reflect the threadsafeness in the class name, since > the usual case is for simple data structures not to be threadsafe. > ThreadsafeSwapQueue? +1 -1 I think that is a good idea assuming we are moving towards a world where nothing is thread safe except for things that begin with "Threadsafe". I think it is a bad idea to have that annotation in the name since we likely will never reach that point, but still can assume nothing about thread safeness... https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:45: : SwapQueue<T>(size) { : queue_(size, initializer) { i.e. don't default-construct, then copy-construct. https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:56: // is used with an input of vectors having the same size. It is confusing that you talk about "input of vectors", yet the argument to the ctor is a pointer to one vector (or should I interpret it as a pointer to a C array of vectors?). Avoid the specific use case when describing how the class works. Use that in an example instead. Seems there's some insight here that I'm missing. Why do we need to make special concessions to handle the case when T is a std::vector? https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:59: using std::swap; Why not just do std::swap on the line below? https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:76: // Note that in the case of T being a vector, if using the above described Please remove these comments. Too much text will make people not read it. https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:112: (last_read_index_ < (queue_.size() - 1) ? last_read_index_ + 1 : 0); I'd rather you last_read_index_++; if (last_read_index_ >= queue_.size()) { last_read_index_ = 0; } here and elsewhere
https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:36: class SwapQueue { On 2015/10/13 07:53:05, solenberg wrote: > On 2015/10/12 14:05:48, kwiberg-webrtc wrote: > > It might be a good idea to reflect the threadsafeness in the class name, since > > the usual case is for simple data structures not to be threadsafe. > > ThreadsafeSwapQueue? > > +1 -1 > > I think that is a good idea assuming we are moving towards a world where nothing > is thread safe except for things that begin with "Threadsafe". > I think it is a bad idea to have that annotation in the name since we likely > will never reach that point, but still can assume nothing about thread > safeness... OK. Then I suggest to err on the side of brevity. https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:56: // is used with an input of vectors having the same size. On 2015/10/13 07:53:05, solenberg wrote: > It is confusing that you talk about "input of vectors", yet the argument to the > ctor is a pointer to one vector (or should I interpret it as a pointer to a C > array of vectors?). Avoid the specific use case when describing how the class > works. Use that in an example instead. > > Seems there's some insight here that I'm missing. Why do we need to make special > concessions to handle the case when T is a std::vector? We don't. std::vector is less convenient than it could be because it needs a constructor argument to make a nonzero-sized vector, and no constructor will create a vector with a given capacity != size. But such is the case for other choices of T too, so I agree it makes more sense to describe it generically in the text and point to an example that uses vectors. https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:59: using std::swap; On 2015/10/13 07:53:05, solenberg wrote: > Why not just do std::swap on the line below? Because the author of T might have defined a swap(T&, T&) that can be found through ADL. Or she might not, in which case we'll end up calling std::swap since we just invited it to our scope. See e.g. http://en.cppreference.com/w/cpp/concept/Swappable, http://en.cppreference.com/w/cpp/algorithm/swap, http://stackoverflow.com/questions/6380862/how-to-provide-a-swap-function-for..., and Item 25 in Effective C++.
https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/BUILD.gn (right): https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/BUILD.gn:91: "swapped_nonblocking_queue.h", On 2015/10/13 06:37:57, hlundin-webrtc wrote: > I wonder if this should be sitting in common_audio instead, together with the > RingBuffer and other friends. I think that makes sense! I will move it, but if anyone objects, please let me know and we can discuss it further. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/swapped_nonblocking_queue.h (right): https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:21: // This class is to be used as a one-way queue where data is On 2015/10/12 14:05:48, kwiberg-webrtc wrote: > On 2015/10/12 13:01:06, peah-webrtc wrote: > > On 2015/10/10 10:16:56, kwiberg-webrtc wrote: > > > "is to be used as a" -> "is" > > > > Sorry, I don't follow that. Please elaborate further :-) > > Just replace the left-hand side string with the right-hand side one. Ah :-). Of course, I thought it was something more fundamental. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:21: // This class is to be used as a one-way queue where data is On 2015/10/12 14:05:48, kwiberg-webrtc wrote: > On 2015/10/12 13:01:06, peah-webrtc wrote: > > On 2015/10/10 10:16:56, kwiberg-webrtc wrote: > > > "is to be used as a" -> "is" > > > > Sorry, I don't follow that. Please elaborate further :-) > > Just replace the left-hand side string with the right-hand side one. Done. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:23: // The read and write operations are protected by a common lock. On 2015/10/13 06:37:57, hlundin-webrtc wrote: > On 2015/10/10 10:16:56, kwiberg-webrtc wrote: > > At this level, it's probably better to just say that it's safe to access this > > concurrently from multiple threads. > > +1. Spare the implementation details. Done. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:24: // The queueable objects must all implement a swap() method. On 2015/10/12 14:05:48, kwiberg-webrtc wrote: > On 2015/10/12 13:01:08, peah-webrtc wrote: > > On 2015/10/10 10:16:55, kwiberg-webrtc wrote: > > > That shouldn't be necessary. As long as the usual swapping idiom > > > (http://en.cppreference.com/w/cpp/concept/Swappable) works, we should be > able > > to > > > use T. > > > > > > But you do require T to have a default constructor. > > > > I changed the comment a bit. Could you please verify that it is correct! > > Looks good. Acknowledged. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:30: Reset(); On 2015/10/13 06:37:57, hlundin-webrtc wrote: > On 2015/10/10 10:16:56, kwiberg-webrtc wrote: > > This takes a lock, and just sets two members to zero. I'd say it makes more > > sense to initialize them without this call. > > OTOH, calling Reset() here makes it more obvious that the Reset method in fact > resets the object to the same state it had when constructed. That was partly my initial intention as well, and also I wanted to avoid code duplication. With the current implementation, and the Clear method, it is a bit different though. Are you fine with the current implementation? https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:37: it != queue_.end(); ++it) On 2015/10/13 06:37:57, hlundin-webrtc wrote: > On 2015/10/09 12:09:54, the sun wrote: > > Always use braces, please. > > I agree, but I know that braces here would get push-back in Chromium. But, > please add them here. Acknowledged. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:39: } On 2015/10/12 14:05:48, kwiberg-webrtc wrote: > On 2015/10/12 13:01:07, peah-webrtc wrote: > > On 2015/10/10 10:16:55, kwiberg-webrtc wrote: > > > std::vector has a constructor for this too. > > > > I removed this one and instead added the one below. Is that ok? > > No, I count three constructors in the new patch set. See comments there. Will have a look at that https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:50: // Returns false if the queue is full, and true if not. On 2015/10/13 06:37:57, hlundin-webrtc wrote: > Clarify that false if returned if the queue is full already before trying to > insert; not if it becomes full by this call. Good point! I added some more documentation regarding that. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:61: input->swap(queue_[last_write_index_]); On 2015/10/12 14:05:48, kwiberg-webrtc wrote: > On 2015/10/12 13:01:07, peah-webrtc wrote: > > On 2015/10/10 10:16:56, kwiberg-webrtc wrote: > > > See http://en.cppreference.com/w/cpp/concept/Swappable for the recommended > way > > > to swap objects of an arbitrary type. In brief, you should do > > > > > > #include <utility> > > > > > > ... > > > > > > using std::swap; > > > swap(x, y); > > > > > > That will work if T has added a specialization to std::swap (or if the > > standard > > > library has one built in), if there is a method T::swap(T&), or if there is > a > > > non-member swap(T&, T&). > > > > Thanks!!!! Does this look as what you intended? > > Yes. Acknowledged. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue.h:67: // Returns false if the queue is empty, and true if not. On 2015/10/13 06:37:57, hlundin-webrtc wrote: > Again, clarify that false is returned if the queue was already empty, so that no > data could be fetched. Good point! I added some more documentation regarding that. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc (right): https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc:2: * Copyright (c) 2014 The WebRTC project authors. All Rights Reserved. On 2015/10/13 06:37:58, hlundin-webrtc wrote: > On 2015/10/10 10:16:57, kwiberg-webrtc wrote: > > You do seem to like multiples of 53... > > I think 53 is just a coincident. 19 is obviously the factor of desire. :-) It is a shame 42 is no prime number. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc:42: explicit Message(int id, int info) { On 2015/10/13 06:37:57, hlundin-webrtc wrote: > No need to make a 2-argument constructor explicit, is it? True! Removed that. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc:112: // Thread callback for the reader thread On 2015/10/13 06:37:57, hlundin-webrtc wrote: > End with . Done. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc:118: // Thread callback for the writer thread On 2015/10/13 06:37:57, hlundin-webrtc wrote: > End with . Done. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc:129: size_t GetNumMessagesRead() { On 2015/10/13 06:37:57, hlundin-webrtc wrote: > const-declare the method (and make crit_ mutable). Done. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc:131: return num_messages_read_; On 2015/10/13 06:37:57, hlundin-webrtc wrote: > I'm not sure, but this seems like the kind of thing that should be able to > achieve with an atomic instead of using locks. One thread is making simple > increments, and the other is reading the value. That is fully true. But I think for the purpose of the test it does not matter. I've minimized the time the lock is held. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc:146: size_t num_messages_read = GetNumMessagesRead(); On 2015/10/13 06:37:58, hlundin-webrtc wrote: > const > But, you don't need this variable, at least not here. Either move it inside the > if-statement below, or simply use the method invocation as index: > messages_[GetNumMessagesRead()] Done. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc:181: SleepMs(rand_r(seed) % (max_sleep + 1)); On 2015/10/13 06:37:57, hlundin-webrtc wrote: > Will rand_r work across platforms? Great find! I implemented a threadsafe random generator instead to both get platform compatibility and void lint warnings about threadsafety of rand(). https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc:186: EXPECT_TRUE(reader_thread_->Start()); On 2015/10/13 06:37:57, hlundin-webrtc wrote: > ASSERT_TRUE. Makes little sense to continue if thread-starting fails. Done. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc:226: queue_size_ = static_cast<size_t>(testing::get<0>(GetParam())); On 2015/10/13 06:37:57, hlundin-webrtc wrote: > If you parse these parameters in the constructor instead of in SetUp, you can > make all these member variables const. Done. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc:231: // llocate read and write buffers. On 2015/10/13 06:37:57, hlundin-webrtc wrote: > -> Allocate Done. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc:268: // Thread callback for the reader thread On 2015/10/13 06:37:57, hlundin-webrtc wrote: > End with . Done. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc:274: // Thread callback for the writer thread On 2015/10/13 06:37:57, hlundin-webrtc wrote: > End with . Done. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc:309: size_t num_samples_read = GetNumSamplesRead(); On 2015/10/13 06:37:57, hlundin-webrtc wrote: > const Done. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc:383: EXPECT_TRUE(reader_thread_->Start()); On 2015/10/13 06:37:57, hlundin-webrtc wrote: > Again, ASSERT_*? Done. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc:407: unsigned int reader_seed = 42; On 2015/10/13 06:37:57, hlundin-webrtc wrote: > Trailing underscore, please. Done. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc:408: unsigned int writer_seed = 37; On 2015/10/13 06:37:58, hlundin-webrtc wrote: > Trailing underscore, please. Done. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc:419: // Test parameters for the message queue tests. On 2015/10/13 06:37:57, hlundin-webrtc wrote: > Let the consts live in the anonymous namespace too. Done. https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/swapped_nonblocking_queue_unittest.cc:453: } // namespace webrtc On 2015/10/10 10:16:57, kwiberg-webrtc wrote: > It might just be me, but I think testing of the queue should focus less on > thread safety (which is almost impossible to test comprehensively anyway) since > that's best established by code review and the annotations you've put in, > especially for a piece of code this small and simple. And more on correctness > issues such as verifying that Insert fails iff the queue is already full, corner > cases such as size == 1, checking that the queue will never do anything to the > Ts except swap them, and so on. I agree, and I add some tests for the corner cases, but keep the existing tests. The current test configuration take 1 second to run. https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:12: #define WEBRTC_MODULES_AUDIO_PROCESSING_SWAPPED_NONBLOCKING_QUEUE_H_ On 2015/10/12 14:05:48, kwiberg-webrtc wrote: > The include guard name needs updating. Done. https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:23: // This class is to be used as a one-way queue where data is On 2015/10/12 14:05:48, kwiberg-webrtc wrote: > It actually *is* a one-way queue. Done. https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:25: // It is safe to access the class concurrently from multiple threads. On 2015/10/13 07:53:05, solenberg wrote: > Will git cl format fix the alignment of comment lines to 80 chars? You mean whether multiline comments are rearranged to fit better on 80 chars? https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:28: // When T is a vector, one way of us using this class is by leveraging that On 2015/10/13 07:53:05, solenberg wrote: > What you describe is the general intention of this design; it is not limited to > vectors. Please describe the general case before talking about a specific case. > Also, an example with pseudo code would likely be useful, or referring to a > specific unit test case that illustrates clearly the use. I fully agree. I revised the description, and added a pseudocode example. Could you please check that it looks what you indented it to be! https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:36: class SwapQueue { On 2015/10/13 07:53:05, solenberg wrote: > On 2015/10/12 14:05:48, kwiberg-webrtc wrote: > > It might be a good idea to reflect the threadsafeness in the class name, since > > the usual case is for simple data structures not to be threadsafe. > > ThreadsafeSwapQueue? > > +1 -1 > > I think that is a good idea assuming we are moving towards a world where nothing > is thread safe except for things that begin with "Threadsafe". > I think it is a bad idea to have that annotation in the name since we likely > will never reach that point, but still can assume nothing about thread > safeness... How about omitting threadsafety in the name, but opening up for this to be explicitly turned on/off in the constructor and having two internal code-paths, one threadsafe and one which is not? https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:38: // Creates a queue of size size. On 2015/10/12 14:05:48, kwiberg-webrtc wrote: > Document that this fills the queue with the specified number of > default-constructed Ts. Done. https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:39: explicit SwapQueue(size_t size) { queue_.resize(size); } On 2015/10/12 14:05:48, kwiberg-webrtc wrote: > explicit SwapQueue(size_t size) : queue_(size) {} Done. https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:45: : SwapQueue<T>(size) { On 2015/10/13 07:53:05, solenberg wrote: > : queue_(size, initializer) { > > i.e. don't default-construct, then copy-construct. Done. https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:47: } On 2015/10/12 14:05:48, kwiberg-webrtc wrote: > "explicit" is only needed for one-argument constructors. > > No <T> needed. > > Call queue_'s constructor with two arguments: size and initializer. > > Don't call the other SwapQueue constructor---that will initialize queue_ twice. > > I don't think the example of T == std::vector in the comment helps. > > And it's probably unnecessary to have this constructor at all; the first and > third one seem sufficient. I removed this constructor. https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:50: // are formed as in the supplied empty slots input On 2015/10/12 14:05:48, kwiberg-webrtc wrote: > Maybe just > > // Create a queue with "empty" Ts from the given > // vector. (The constructor will steal the contents from > // the supplied vector, leaving it empty.) > > This probably works best in conjunction with a longer explanation of "full" and > "empty" Ts, maybe in the class description. Excellent suggestion. https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:50: // are formed as in the supplied empty slots input On 2015/10/12 14:05:48, kwiberg-webrtc wrote: > Maybe just > > // Create a queue with "empty" Ts from the given > // vector. (The constructor will steal the contents from > // the supplied vector, leaving it empty.) > > This probably works best in conjunction with a longer explanation of "full" and > "empty" Ts, maybe in the class description. Done. https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:52: // standard library. On 2015/10/12 15:39:10, kwiberg-webrtc wrote: > Actually, I realized on the way home that we only need the language part of > C++11. Like this: > > explicit SwapQueue(std::vector<T>&& empty_slots) { > RTC_CHECK_GE(empty_slots->size(), 1u); > using std::swap; > swap(queue_, empty_slots); > } > > To create a queue with n default-constructed "empty" slots: > > SwapQueue<T>(std::vector<T>(n)); > > With n "empty" slots copy-constructed from t: > > SwapQueue<T>(std::vector<T>(n, t)); > > With some more elaborately constructed vector of Ts: > > std::vector<T> vt; > // Fill it! > SwapQueue<T>(static_cast<std::vector<T>&&>(vt)); > > The C++11 standard library would let us use std::move instead of a cast in the > third example, but that's all. Nice! But I get a cl upload warning that the below passing of the references is not allowed, so therefore I did not add this. Ok? https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:56: // is used with an input of vectors having the same size. On 2015/10/12 14:05:48, kwiberg-webrtc wrote: > You repeat this several times. Maybe just once, in the class description? > > I think it might be useful to phrase it in terms of "full" and "empty" Ts: The > Ts passed to (or created by) the queue's constructor are "empty". Insert swaps a > "full" T for an "empty" T, and Remove does the opposite. Except for in the > constructors and the destructor, the queue never creates or destroys Ts, just > swaps them, so it's guaranteed that the "empty" Ts you get from Insert are the > same as the "empty" Ts given to Remove (or passed to/created by the > constructors). It follows that if you want the "empty" Ts returned by Insert to > have some property such as a certain size, just ensure that the Ts you give to > the constructor and Remove have that property. > > Clear is a special case; it moves all Ts currently in the "full" bucket to the > "empty" bucket. This makes the queue harder to explain, which is one reason it'd > be nice to not have it. > > Another thing that would make the queue easier to explain would be if you had > only the third constructor, so that you could claim simply that it never creates > Ts. I agree about repetition, and I removed that part.¸I also revised the class description quite a lot but I could not really fit the above scheme well. Could you please check to see whether you think it is ok! https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:56: // is used with an input of vectors having the same size. On 2015/10/13 09:59:53, kwiberg-webrtc wrote: > On 2015/10/13 07:53:05, solenberg wrote: > > It is confusing that you talk about "input of vectors", yet the argument to > the > > ctor is a pointer to one vector (or should I interpret it as a pointer to a C > > array of vectors?). Avoid the specific use case when describing how the class > > works. Use that in an example instead. > > > > Seems there's some insight here that I'm missing. Why do we need to make > special > > concessions to handle the case when T is a std::vector? > > We don't. std::vector is less convenient than it could be because it needs a > constructor argument to make a nonzero-sized vector, and no constructor will > create a vector with a given capacity != size. But such is the case for other > choices of T too, so I agree it makes more sense to describe it generically in > the text and point to an example that uses vectors. I made an attempt to do that. Could you please have a look at the current comment scheme! https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:59: using std::swap; On 2015/10/13 07:53:05, solenberg wrote: > Why not just do std::swap on the line below? I kept it as it is for now. Please let me know if you are happy with the change. https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:59: using std::swap; On 2015/10/13 09:59:53, kwiberg-webrtc wrote: > On 2015/10/13 07:53:05, solenberg wrote: > > Why not just do std::swap on the line below? > > Because the author of T might have defined a swap(T&, T&) that can be found > through ADL. Or she might not, in which case we'll end up calling std::swap > since we just invited it to our scope. > > See e.g. http://en.cppreference.com/w/cpp/concept/Swappable, > http://en.cppreference.com/w/cpp/algorithm/swap, > http://stackoverflow.com/questions/6380862/how-to-provide-a-swap-function-for..., > and Item 25 in Effective C++. Acknowledged. https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:71: // Inserts a chunk of data into the queue in a thread safe manner. On 2015/10/12 14:05:48, kwiberg-webrtc wrote: > Document thread safety once, in the class description. Every operation should be > safe. Done. https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:74: // Returns false if the queue is full, and true if not. On 2015/10/12 14:05:48, kwiberg-webrtc wrote: > Document that the operation is a no-op iff it returns false. Done. https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:76: // Note that in the case of T being a vector, if using the above described On 2015/10/13 07:53:05, solenberg wrote: > Please remove these comments. Too much text will make people not read it. Done. https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:98: // Returns false if the queue is empty, and true if not. On 2015/10/12 14:05:48, kwiberg-webrtc wrote: > Same as above: document thread safety once, and say it's a no-op iff it returns > false. Done. https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:112: (last_read_index_ < (queue_.size() - 1) ? last_read_index_ + 1 : 0); On 2015/10/13 07:53:05, solenberg wrote: > I'd rather you > > last_read_index_++; > if (last_read_index_ >= queue_.size()) { > last_read_index_ = 0; > } > > here and elsewhere Done. https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:113: output->swap(queue_[last_read_index_]); On 2015/10/12 14:05:48, kwiberg-webrtc wrote: > Use the same swap idiom as above. Done. https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:123: size_t num_elements_ GUARDED_BY(crit_queue_) = 0; On 2015/10/12 14:05:48, kwiberg-webrtc wrote: > Document the invariants: queue_.size() never changes, 0 <= num_elements_ <= > queue_.size(), last_read_index_ + num_elements_ = last_write_index_ mod > queue_.size() (with the two indices equal iff the queue is full or empty). > > And note that by using last_*_index instead of next_*_index, you use the slots > in the slightly inelegant order 1, 2, 3, ..., size-1, 0, 1, 2, ... Good point! I changed to using next, and added the comments. https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:123: size_t num_elements_ GUARDED_BY(crit_queue_) = 0; On 2015/10/12 14:05:48, kwiberg-webrtc wrote: > Document the invariants: queue_.size() never changes, 0 <= num_elements_ <= > queue_.size(), last_read_index_ + num_elements_ = last_write_index_ mod > queue_.size() (with the two indices equal iff the queue is full or empty). > > And note that by using last_*_index instead of next_*_index, you use the slots > in the slightly inelegant order 1, 2, 3, ..., size-1, 0, 1, 2, ... Done.
https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/BUILD.gn (right): https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/BUILD.gn:91: "swapped_nonblocking_queue.h", On 2015/10/13 11:56:11, peah-webrtc wrote: > On 2015/10/13 06:37:57, hlundin-webrtc wrote: > > I wonder if this should be sitting in common_audio instead, together with the > > RingBuffer and other friends. > > I think that makes sense! I will move it, but if anyone objects, please let me > know and we can discuss it further. It might be generic enough to merit a place in webrtc/base, since it's not audio-specific. https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:36: class SwapQueue { On 2015/10/13 11:56:12, peah-webrtc wrote: > On 2015/10/13 07:53:05, solenberg wrote: > > On 2015/10/12 14:05:48, kwiberg-webrtc wrote: > > > It might be a good idea to reflect the threadsafeness in the class name, > since > > > the usual case is for simple data structures not to be threadsafe. > > > ThreadsafeSwapQueue? > > > > +1 -1 > > > > I think that is a good idea assuming we are moving towards a world where > nothing > > is thread safe except for things that begin with "Threadsafe". > > I think it is a bad idea to have that annotation in the name since we likely > > will never reach that point, but still can assume nothing about thread > > safeness... > > How about omitting threadsafety in the name, but opening up for this to be > explicitly turned on/off in the constructor and having two internal code-paths, > one threadsafe and one which is not? Dynamically taking locks or not would make the code more complicated than necessary. I'd suggest locking unconditionally like you do now, or possibly passing the mutex type as a second (optional) template parameter. But... no, just keep the current code until you see a need for a non-threadsafe variant.
On 2015/10/13 12:09:07, kwiberg-webrtc wrote: > https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... > File webrtc/modules/audio_processing/BUILD.gn (right): > > https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... > webrtc/modules/audio_processing/BUILD.gn:91: "swapped_nonblocking_queue.h", > On 2015/10/13 11:56:11, peah-webrtc wrote: > > On 2015/10/13 06:37:57, hlundin-webrtc wrote: > > > I wonder if this should be sitting in common_audio instead, together with > the > > > RingBuffer and other friends. > > > > I think that makes sense! I will move it, but if anyone objects, please let me > > know and we can discuss it further. > > It might be generic enough to merit a place in webrtc/base, since it's not > audio-specific. > > https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... > File webrtc/modules/audio_processing/swap_queue.h (right): > > https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/swap_queue.h:36: class SwapQueue { > On 2015/10/13 11:56:12, peah-webrtc wrote: > > On 2015/10/13 07:53:05, solenberg wrote: > > > On 2015/10/12 14:05:48, kwiberg-webrtc wrote: > > > > It might be a good idea to reflect the threadsafeness in the class name, > > since > > > > the usual case is for simple data structures not to be threadsafe. > > > > ThreadsafeSwapQueue? > > > > > > +1 -1 > > > > > > I think that is a good idea assuming we are moving towards a world where > > nothing > > > is thread safe except for things that begin with "Threadsafe". > > > I think it is a bad idea to have that annotation in the name since we likely > > > will never reach that point, but still can assume nothing about thread > > > safeness... > > > > How about omitting threadsafety in the name, but opening up for this to be > > explicitly turned on/off in the constructor and having two internal > code-paths, > > one threadsafe and one which is not? > > Dynamically taking locks or not would make the code more complicated than > necessary. I'd suggest locking unconditionally like you do now, or possibly > passing the mutex type as a second (optional) template parameter. But... no, > just keep the current code until you see a need for a non-threadsafe variant. +1
https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/BUILD.gn (right): https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/BUILD.gn:91: "swapped_nonblocking_queue.h", On 2015/10/13 12:09:07, kwiberg-webrtc wrote: > On 2015/10/13 11:56:11, peah-webrtc wrote: > > On 2015/10/13 06:37:57, hlundin-webrtc wrote: > > > I wonder if this should be sitting in common_audio instead, together with > the > > > RingBuffer and other friends. > > > > I think that makes sense! I will move it, but if anyone objects, please let me > > know and we can discuss it further. > > It might be generic enough to merit a place in webrtc/base, since it's not > audio-specific. Yes, in time. I think first we let it get some mileage within our little world. https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:25: // It is safe to access the class concurrently from multiple threads. On 2015/10/13 11:56:13, peah-webrtc wrote: > On 2015/10/13 07:53:05, solenberg wrote: > > Will git cl format fix the alignment of comment lines to 80 chars? > > You mean whether multiline comments are rearranged to fit better on 80 chars? yes https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:47: } On 2015/10/13 11:56:13, peah-webrtc wrote: > On 2015/10/12 14:05:48, kwiberg-webrtc wrote: > > "explicit" is only needed for one-argument constructors. > > > > No <T> needed. > > > > Call queue_'s constructor with two arguments: size and initializer. > > > > Don't call the other SwapQueue constructor---that will initialize queue_ > twice. > > > > I don't think the example of T == std::vector in the comment helps. > > > > And it's probably unnecessary to have this constructor at all; the first and > > third one seem sufficient. > > I removed this constructor. But why? Supplying a prototype object which is copied to each slot sounds less error prone than relying on the client to construct the full vector used in the queue (as with the ctor below). https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:59: using std::swap; On 2015/10/13 11:56:13, peah-webrtc wrote: > On 2015/10/13 09:59:53, kwiberg-webrtc wrote: > > On 2015/10/13 07:53:05, solenberg wrote: > > > Why not just do std::swap on the line below? > > > > Because the author of T might have defined a swap(T&, T&) that can be found > > through ADL. Or she might not, in which case we'll end up calling std::swap > > since we just invited it to our scope. > > > > See e.g. http://en.cppreference.com/w/cpp/concept/Swappable, > > http://en.cppreference.com/w/cpp/algorithm/swap, > > > http://stackoverflow.com/questions/6380862/how-to-provide-a-swap-function-for..., > > and Item 25 in Effective C++. > > Acknowledged. Great! Thanks for the explanation. https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... File webrtc/common_audio/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:31: // SwapQueue<int> q(std::vector<int>(2, 0)); // q contains [0 0]; You can't do this with the class as it is defined in this patch set. https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:46: // to be of a special size, care must be taken that the queue is initialized This has nothing to do with "size". If the caller relies on *any* property of the objects returned, it must make sure that: 1. the queue is initialized with objects with such properties 2. any objects supplied to insert or remove also have such properties. I think we should add an invariant-check callback functor so that we can RTC_DCHECK() that the expected conditions are always met. WDYT? https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:82: // Create a queue with "empty" Ts from the given It does not create a queue with empty Ts. It creates a queue that has precisely the contents of the given "prototype" vector. https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:88: // If used when T is a vector and the above described pattern that ensures Remove this comment, it is confusing https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:93: RTC_CHECK_GE(empty_slots->size(), 1u); That's not actually a requirement. The code works anyway. https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:114: if (num_elements_ == queue_.size()) Ohh, braces, please...
https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... File webrtc/common_audio/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:12: #define WEBRTC_MODULES_SWAPPED_QUEUE_H_ Still behind the times! https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:30: // Pseudocode example of queue usage: What's pseudo about this code? https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:49: // dynamic. A pseudocode example of this is shown below where no vector resizing Instead of talking about the size of the Ts, just point out that the Ts you get out are the same ones you put in in Remove and the constructor. The last sentence ("A pseudocode example...") then establishes what this means for Ts with variable-size heap allocations. https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:52: // Pseudocode example of queue usage with vector: Why pseudo? https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:58: // v_init[k].resize(kChunkSize); Use a for (auto& v : v_init) loop. That will initialize the whole vector, not just the first two elements, but that might be OK? https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:65: // delayed_output.resize(kChunkSize); std::vector<int> input(kChunkSize); std::vector<int> output(kChunkSize); https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:70: // read_values = function_that_copies_ints_from_file_into_vector(&input); read_samples or read_values? https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:74: // } This example is probably a bit too long to clearly illustrate the point, and doesn't reflect the intended real-world usage. It might be better to make a real unit test with a toy producer and a toy consumer, and have EXPECTs on the vectors' .capacity() that ensure that no run-time reallocation of vectors takes place. https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:86: // standard library. This turned out to not be allowed by the relevant style guides, so remove the TODO. https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:91: // size. Remove this paragraph. The class documentation already explains why you'd conceivably want non-default-constructed Ts in the "empty" slots. https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:106: // Inserts a chunk of data into the queue. The input is swapped with the next "chunk of data" -> "T" https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:107: // available value of type T present in the queue. This doesn't make it clear that the queue makes a sharp distinction between "empty" and "full" Ts; on the contrary, it suggests that you swap out a previously inserted T. Maybe ...is swapped with the least recently inserted "empty" T. But that requires the class doc to have introduced "empty" and "full", or something equivalent. https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:108: // Returns false if the queue is full (in which case no insert is performed), "is full" -> "was already full" https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:113: // Early return if the queue is full. This comment is not necessary. https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:131: // and true if not. See the three comments on the Insert doc. https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:135: // Early return for empty queue. Remove comment. https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:155: // next_write_index_ Also, they're both in the range [0,size). https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... File webrtc/common_audio/swap_queue_unittest.cc (right): https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue_unittest.cc:28: class ThreadSafeRandomNumberGenerator { It's not thread safe, it just avoids being terribly thread-unsafe by not having any global mutable state... https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue_unittest.cc:55: } Why do you need to support several isers? Couldn't you just give them their own instances? https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue_unittest.cc:69: }; Wouldn't it be simpler to just protect rand() calls with a lock?
https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:47: } On 2015/10/13 12:34:26, the sun wrote: > On 2015/10/13 11:56:13, peah-webrtc wrote: > > On 2015/10/12 14:05:48, kwiberg-webrtc wrote: > > > "explicit" is only needed for one-argument constructors. > > > > > > No <T> needed. > > > > > > Call queue_'s constructor with two arguments: size and initializer. > > > > > > Don't call the other SwapQueue constructor---that will initialize queue_ > > twice. > > > > > > I don't think the example of T == std::vector in the comment helps. > > > > > > And it's probably unnecessary to have this constructor at all; the first and > > > third one seem sufficient. > > > > I removed this constructor. > > But why? Supplying a prototype object which is copied to each slot sounds less > error prone than relying on the client to construct the full vector used in the > queue (as with the ctor below). Sure, it might be a convenient shortcut. https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... File webrtc/common_audio/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:46: // to be of a special size, care must be taken that the queue is initialized On 2015/10/13 12:34:27, the sun wrote: > This has nothing to do with "size". If the caller relies on *any* property of > the objects returned, it must make sure that: > 1. the queue is initialized with objects with such properties > 2. any objects supplied to insert or remove also have such properties. > > I think we should add an invariant-check callback functor so that we can > RTC_DCHECK() that the expected conditions are always met. WDYT? Yes, we should definitely do that. (And by "we", we obviously mean you, Per. Fredrik and I are much too busy telling you what you're doing wrong.) https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:114: if (num_elements_ == queue_.size()) On 2015/10/13 12:34:27, the sun wrote: > Ohh, braces, please... But they just make the code harder to read...
https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... File webrtc/common_audio/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:46: // to be of a special size, care must be taken that the queue is initialized On 2015/10/13 14:08:36, kwiberg-webrtc wrote: > On 2015/10/13 12:34:27, the sun wrote: > > This has nothing to do with "size". If the caller relies on *any* property of > > the objects returned, it must make sure that: > > 1. the queue is initialized with objects with such properties > > 2. any objects supplied to insert or remove also have such properties. > > > > I think we should add an invariant-check callback functor so that we can > > RTC_DCHECK() that the expected conditions are always met. WDYT? > > Yes, we should definitely do that. (And by "we", we obviously mean you, Per. > Fredrik and I are much too busy telling you what you're doing wrong.) Evidently, given the number of comments we've managed to produce. Btw, the predicate should be an optional template argument so that the check is considered part of the queue type. https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:114: if (num_elements_ == queue_.size()) On 2015/10/13 14:08:36, kwiberg-webrtc wrote: > On 2015/10/13 12:34:27, the sun wrote: > > Ohh, braces, please... > > But they just make the code harder to read... No, they make sure the reader isn't ever fooled by messed up indentation (and makes it less easy to introduce cut+paste bugs).
https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... File webrtc/common_audio/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:12: #define WEBRTC_MODULES_SWAPPED_QUEUE_H_ On 2015/10/13 13:54:23, kwiberg-webrtc wrote: > Still behind the times! Ouch, good find! https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:12: #define WEBRTC_MODULES_SWAPPED_QUEUE_H_ On 2015/10/13 13:54:23, kwiberg-webrtc wrote: > Still behind the times! Done. https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:30: // Pseudocode example of queue usage: On 2015/10/13 13:54:23, kwiberg-webrtc wrote: > What's pseudo about this code? :-) Removed pseudo https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:30: // Pseudocode example of queue usage: On 2015/10/13 13:54:23, kwiberg-webrtc wrote: > What's pseudo about this code? Done. https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:31: // SwapQueue<int> q(std::vector<int>(2, 0)); // q contains [0 0]; On 2015/10/13 12:34:27, the sun wrote: > You can't do this with the class as it is defined in this patch set. Fully correct! I changed it a bit to signify that we initially have undefined values in the queue. https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:46: // to be of a special size, care must be taken that the queue is initialized On 2015/10/13 14:19:45, solenberg wrote: > On 2015/10/13 14:08:36, kwiberg-webrtc wrote: > > On 2015/10/13 12:34:27, the sun wrote: > > > This has nothing to do with "size". If the caller relies on *any* property > of > > > the objects returned, it must make sure that: > > > 1. the queue is initialized with objects with such properties > > > 2. any objects supplied to insert or remove also have such properties. > > > > > > I think we should add an invariant-check callback functor so that we can > > > RTC_DCHECK() that the expected conditions are always met. WDYT? > > > > Yes, we should definitely do that. (And by "we", we obviously mean you, Per. > > Fredrik and I are much too busy telling you what you're doing wrong.) > > Evidently, given the number of comments we've managed to produce. > > Btw, the predicate should be an optional template argument so that the check is > considered part of the queue type. Revised according to discussion. https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:49: // dynamic. A pseudocode example of this is shown below where no vector resizing On 2015/10/13 13:54:23, kwiberg-webrtc wrote: > Instead of talking about the size of the Ts, just point out that the Ts you get > out are the same ones you put in in Remove and the constructor. The last > sentence ("A pseudocode example...") then establishes what this means for Ts > with variable-size heap allocations. I rewrote and rephrased, could you please have a look again! https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:52: // Pseudocode example of queue usage with vector: On 2015/10/13 13:54:23, kwiberg-webrtc wrote: > Why pseudo? Done. https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:58: // v_init[k].resize(kChunkSize); On 2015/10/13 13:54:23, kwiberg-webrtc wrote: > Use a for (auto& v : v_init) loop. That will initialize the whole vector, not > just the first two elements, but that might be OK? Rewrote this. https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:65: // delayed_output.resize(kChunkSize); On 2015/10/13 13:54:23, kwiberg-webrtc wrote: > std::vector<int> input(kChunkSize); > std::vector<int> output(kChunkSize); Done. https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:70: // read_values = function_that_copies_ints_from_file_into_vector(&input); On 2015/10/13 13:54:23, kwiberg-webrtc wrote: > read_samples or read_values? Done. https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:74: // } On 2015/10/13 13:54:24, kwiberg-webrtc wrote: > This example is probably a bit too long to clearly illustrate the point, and > doesn't reflect the intended real-world usage. It might be better to make a real > unit test with a toy producer and a toy consumer, and have EXPECTs on the > vectors' .capacity() that ensure that no run-time reallocation of vectors takes > place. I have that as well, should I just refer to that instead and remove this example? https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:82: // Create a queue with "empty" Ts from the given On 2015/10/13 12:34:27, the sun wrote: > It does not create a queue with empty Ts. It creates a queue that has precisely > the contents of the given "prototype" vector. Done. https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:86: // standard library. On 2015/10/13 13:54:23, kwiberg-webrtc wrote: > This turned out to not be allowed by the relevant style guides, so remove the > TODO. Done. https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:91: // size. On 2015/10/13 13:54:23, kwiberg-webrtc wrote: > Remove this paragraph. The class documentation already explains why you'd > conceivably want non-default-constructed Ts in the "empty" slots. Done. https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:93: RTC_CHECK_GE(empty_slots->size(), 1u); On 2015/10/13 12:34:27, the sun wrote: > That's not actually a requirement. The code works anyway. Done. https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:106: // Inserts a chunk of data into the queue. The input is swapped with the next On 2015/10/13 13:54:23, kwiberg-webrtc wrote: > "chunk of data" -> "T" Done. https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:107: // available value of type T present in the queue. On 2015/10/13 13:54:23, kwiberg-webrtc wrote: > This doesn't make it clear that the queue makes a sharp distinction between > "empty" and "full" Ts; on the contrary, it suggests that you swap out a > previously inserted T. Maybe > > ...is swapped with the least recently inserted "empty" T. > > But that requires the class doc to have introduced "empty" and "full", or > something equivalent. I agree that the full/empty description simplifies things. I rewrote the documentation accordingly. Please check to see what you think about it! https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:108: // Returns false if the queue is full (in which case no insert is performed), On 2015/10/13 13:54:24, kwiberg-webrtc wrote: > "is full" -> "was already full" Done. https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:113: // Early return if the queue is full. On 2015/10/13 13:54:23, kwiberg-webrtc wrote: > This comment is not necessary. Done. https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:114: if (num_elements_ == queue_.size()) On 2015/10/13 14:19:45, solenberg wrote: > On 2015/10/13 14:08:36, kwiberg-webrtc wrote: > > On 2015/10/13 12:34:27, the sun wrote: > > > Ohh, braces, please... > > > > But they just make the code harder to read... > > No, they make sure the reader isn't ever fooled by messed up indentation (and > makes it less easy to introduce cut+paste bugs). Added braces :-) https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:131: // and true if not. On 2015/10/13 13:54:23, kwiberg-webrtc wrote: > See the three comments on the Insert doc. Done. https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:135: // Early return for empty queue. On 2015/10/13 13:54:23, kwiberg-webrtc wrote: > Remove comment. Done. https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:155: // next_write_index_ On 2015/10/13 13:54:23, kwiberg-webrtc wrote: > Also, they're both in the range [0,size). Done. https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... File webrtc/common_audio/swap_queue_unittest.cc (right): https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue_unittest.cc:28: class ThreadSafeRandomNumberGenerator { On 2015/10/13 13:54:24, kwiberg-webrtc wrote: > It's not thread safe, it just avoids being terribly thread-unsafe by not having > any global mutable state... Fully true, I rewrote it using a functor instead. https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue_unittest.cc:55: } On 2015/10/13 13:54:24, kwiberg-webrtc wrote: > Why do you need to support several isers? Couldn't you just give them their own > instances? Fully true, I rewrote it using a functor instead. https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue_unittest.cc:69: }; On 2015/10/13 13:54:24, kwiberg-webrtc wrote: > Wouldn't it be simpler to just protect rand() calls with a lock? Fully true! This is definitely over-engineered! I rewrote it using a functor instead.
https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... File webrtc/common_audio/swap_queue_unittest.cc (right): https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue_unittest.cc:35: random_numbers_[k] = rand(); Note that it doesn't matter that we make rand() thread safe here. Some other thread could be using it anyway. There's a simple PRNG class in the BWE test framework that we could possibly move into webrtc/test/ and reuse. It doesn't even need a lock, since then each thread could have its own instance. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... File webrtc/common_audio/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:60: // Default item invariance verifier callback function. Please use the term "invariant" as that is more common in programming than invariance (which is more common in the world of signals...) https://www.google.se/search?q=invariant+definition&oq=invariant also, prepend "SwapQueue" since this is in the vast webrtc:: namespace. https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:70: bool (*ItemInvarianceVerifier)(const T&) = I think CheckItemInvariant was a better name... https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:91: // Inserts a Y into the queue. The "full" input is swapped with the "empty" Y -> T "Insert a T into the queue by swapping *input with an element already in the queue. Returns true if the item was inserted or false if not (the queue was full). The T given in *input must pass a CheckItemInvariant() test. The contents of *input after the call are guaranteed to also pass a CheckItemInvariant() test." https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:101: if (num_elements_ == queue_.size()) { Thank you! :) https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:117: // Removes a chunk of data from the queue. The "full" queue element is swapped "Remove a T from the queue by swapping *output with an element in the queue. Returns true if an item could be removed or false if not (the queue was empty). The T given in *output must pass a CheckItemInvariant() test."
https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... File webrtc/common_audio/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:74: // } On 2015/10/14 12:16:47, peah-webrtc wrote: > On 2015/10/13 13:54:24, kwiberg-webrtc wrote: > > This example is probably a bit too long to clearly illustrate the point, and > > doesn't reflect the intended real-world usage. It might be better to make a > real > > unit test with a toy producer and a toy consumer, and have EXPECTs on the > > vectors' .capacity() that ensure that no run-time reallocation of vectors > takes > > place. > > I have that as well, should I just refer to that instead and remove this > example? Yes, provided it's small and clean enough to work as an illustrative example. https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... File webrtc/common_audio/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:12: #define WEBRTC_MODULES_SWAP_QUEUE_H_ Don't worry, you'll get it right one of these days! https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:26: // The queueable objects must all be swappable as the Insert and Remove methods "queuable" -> "enqueued", maybe? https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:34: // in the queue is "empty". Perhaps still a bit too imprecise. How about something like this: // Conceptually, the queue maintains an ordered sequence and an unordered bunch // of Ts. At all times, the number of Ts in the sequence and the bunch combined // is N, a constant set in the constructor call. The following operations // modify the sequence and the bunch: // // - The constructor populates the bunch with N Ts (either by // default-constructing them or copy-constructing them from a supplied // prototype T), and leaves the sequence empty. // // - Insert() steals the given T and puts it last in the sequence; it gives // back an arbitrary T from the bunch. (Accomplished with a single swap, by // reassigning the physical location of the removed bunch T to the // sequence.) // // - Remove() steals the given T and puts it in the bunch; it gives back the // first element in the sequence. (Accomplished with a single swap, by // reassigning the physical location of the removed sequence T to the // bunch.) // // - Clean() moves all Ts from the sequence into the bunch. (This is done // without touching any Ts, by reassigning their physical locations.) // // The intended use is for one client (the producer) to fill a T with useful // stuff, and use Insert() to swap it for a T that doesn't contain useful // stuff; and for another client (the consumer) to use Remove() to swap a T // that doesn't contain useful stuff for a T that does. The point of passing Ts // that don't contain anything useful in the reverse direction is to avoid // having to make heap allocations at the producer and releasing them at the // consumer. Note that I didn't reveal the implementation detail that the "bunch" is also an ordered sequence; this in turn allowed me to talk about just "the sequence" and "the bunch" instead of, say, "the 'full' sequence" and "the 'empty' sequence". https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:60: // Default item invariance verifier callback function. On 2015/10/14 13:05:21, the sun wrote: > Please use the term "invariant" as that is more common in programming than > invariance (which is more common in the world of signals...) > https://www.google.se/search?q=invariant+definition&oq=invariant > > also, prepend "SwapQueue" since this is in the vast webrtc:: namespace. You could conceivably also put it in its own namespace, e.g. "internal". That's the suggestion I got for webrtc/base/buffer.h. https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:71: DefaultItemInvarianceVerifier> Not too happy with this name. Maybe simply "ItemChecker"? https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:78: // Creates a queue of size and fills it with copies of prototype. "size" -> "size size". https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:91: // Inserts a Y into the queue. The "full" input is swapped with the "empty" "Y" -> "T" https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:101: if (num_elements_ == queue_.size()) { On 2015/10/14 13:05:21, the sun wrote: > Thank you! :) Aww! Don't give in just because he's more intimidating than I am! https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:117: // Removes a chunk of data from the queue. The "full" queue element is swapped "chunk of data" -> "T" https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:145: void CheckQueueInvariance() { invariance -> invariants. Otherwise, it sounds as if the whole queue is invariant, i.e. constant. Also, queue -> element, since that's what it checks. End result: CheckElementInvariants. https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... File webrtc/common_audio/swap_queue_unittest.cc (right): https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue_unittest.cc:27: // Provides thread_safe random numbers. The comment says nothing that the class name doesn't already say. https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue_unittest.cc:418: EXPECT_DEATH(queue.Insert(&invalid_chunk), ""); Unless your employee ID is 007, you don't get to live twice.
https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... File webrtc/common_audio/swap_queue_unittest.cc (right): https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue_unittest.cc:35: random_numbers_[k] = rand(); On 2015/10/14 13:05:21, the sun wrote: > Note that it doesn't matter that we make rand() thread safe here. Some other > thread could be using it anyway. > > There's a simple PRNG class in the BWE test framework that we could possibly > move into webrtc/test/ and reuse. It doesn't even need a lock, since then each > thread could have its own instance. > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... You are fully correct! If ok with you, I'll keep the functor implementation and use rand in that. That removes the warning at least. https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... File webrtc/common_audio/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:12: #define WEBRTC_MODULES_SWAP_QUEUE_H_ On 2015/10/14 14:10:16, kwiberg-webrtc wrote: > Don't worry, you'll get it right one of these days! :-) https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:12: #define WEBRTC_MODULES_SWAP_QUEUE_H_ On 2015/10/14 14:10:16, kwiberg-webrtc wrote: > Don't worry, you'll get it right one of these days! Done. https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:26: // The queueable objects must all be swappable as the Insert and Remove methods On 2015/10/14 14:10:16, kwiberg-webrtc wrote: > "queuable" -> "enqueued", maybe? Used a slightly different wording. https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:34: // in the queue is "empty". On 2015/10/14 14:10:16, kwiberg-webrtc wrote: > Perhaps still a bit too imprecise. How about something like this: > > // Conceptually, the queue maintains an ordered sequence and an unordered bunch > // of Ts. At all times, the number of Ts in the sequence and the bunch combined > // is N, a constant set in the constructor call. The following operations > // modify the sequence and the bunch: > // > // - The constructor populates the bunch with N Ts (either by > // default-constructing them or copy-constructing them from a supplied > // prototype T), and leaves the sequence empty. > // > // - Insert() steals the given T and puts it last in the sequence; it gives > // back an arbitrary T from the bunch. (Accomplished with a single swap, by > // reassigning the physical location of the removed bunch T to the > // sequence.) > // > // - Remove() steals the given T and puts it in the bunch; it gives back the > // first element in the sequence. (Accomplished with a single swap, by > // reassigning the physical location of the removed sequence T to the > // bunch.) > // > // - Clean() moves all Ts from the sequence into the bunch. (This is done > // without touching any Ts, by reassigning their physical locations.) > // > // The intended use is for one client (the producer) to fill a T with useful > // stuff, and use Insert() to swap it for a T that doesn't contain useful > // stuff; and for another client (the consumer) to use Remove() to swap a T > // that doesn't contain useful stuff for a T that does. The point of passing Ts > // that don't contain anything useful in the reverse direction is to avoid > // having to make heap allocations at the producer and releasing them at the > // consumer. > > Note that I didn't reveal the implementation detail that the "bunch" > is also an ordered sequence; this in turn allowed me to talk about > just "the sequence" and "the bunch" instead of, say, "the 'full' > sequence" and "the 'empty' sequence". This looks super. I used it as it is, and added some additional documentation before and after it. Please behold. https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:60: // Default item invariance verifier callback function. On 2015/10/14 14:10:16, kwiberg-webrtc wrote: > On 2015/10/14 13:05:21, the sun wrote: > > Please use the term "invariant" as that is more common in programming than > > invariance (which is more common in the world of signals...) > > https://www.google.se/search?q=invariant+definition&oq=invariant > > > > also, prepend "SwapQueue" since this is in the vast webrtc:: namespace. > > You could conceivably also put it in its own namespace, e.g. "internal". That's > the suggestion I got for webrtc/base/buffer.h. Done. https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:60: // Default item invariance verifier callback function. On 2015/10/14 13:05:21, the sun wrote: > Please use the term "invariant" as that is more common in programming than > invariance (which is more common in the world of signals...) > https://www.google.se/search?q=invariant+definition&oq=invariant > > also, prepend "SwapQueue" since this is in the vast webrtc:: namespace. I'm still not fully happy with invariant. I change it to verifier, does that sound ok? https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:70: bool (*ItemInvarianceVerifier)(const T&) = On 2015/10/14 13:05:21, the sun wrote: > I think CheckItemInvariant was a better name... Hmm, I'm not either superhappy about the current name. I'll discuss it more in the comment below. https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:71: DefaultItemInvarianceVerifier> On 2015/10/14 14:10:16, kwiberg-webrtc wrote: > Not too happy with this name. Maybe simply "ItemChecker"? Me neither. I changed it now to ItemVerifier. In my mind, the function should verify the properties of the items. ItemChecker is close to that but I think it is a bit more vague. CheckItemInvariant I think is a bit narrow since there is actually not any strict requirement that the items should be invariant. Any opinions or further suggestions? https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:78: // Creates a queue of size and fills it with copies of prototype. On 2015/10/14 14:10:16, kwiberg-webrtc wrote: > "size" -> "size size". Done. https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:91: // Inserts a Y into the queue. The "full" input is swapped with the "empty" On 2015/10/14 13:05:21, the sun wrote: > Y -> T > > "Insert a T into the queue by swapping *input with an element already in the > queue. > Returns true if the item was inserted or false if not (the queue was full). > The T given in *input must pass a CheckItemInvariant() test. > The contents of *input after the call are guaranteed to also pass a > CheckItemInvariant() test." > Looks super. I did some minor editing, in particular about that the test is optional. Please behold! https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:91: // Inserts a Y into the queue. The "full" input is swapped with the "empty" On 2015/10/14 14:10:16, kwiberg-webrtc wrote: > "Y" -> "T" Done. https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:101: if (num_elements_ == queue_.size()) { On 2015/10/14 13:05:21, the sun wrote: > Thank you! :) :D https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:101: if (num_elements_ == queue_.size()) { On 2015/10/14 14:10:16, kwiberg-webrtc wrote: > On 2015/10/14 13:05:21, the sun wrote: > > Thank you! :) > > Aww! Don't give in just because he's more intimidating than I am! This is tricky, and I'm a bit split in this. I've been bashed through many years with that braces were needed and required for security reasons and I definitely agree that they are safer and avoids python-style indentation errors. However,...., I personally prefer compact code, and therefore definitely I prefer not to use braces. https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:117: // Removes a chunk of data from the queue. The "full" queue element is swapped On 2015/10/14 13:05:21, the sun wrote: > "Remove a T from the queue by swapping *output with an element in the queue. > Returns true if an item could be removed or false if not (the queue was empty). > The T given in *output must pass a CheckItemInvariant() test." Nice! I changed to that, but revised it a bit. Please behold! https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:117: // Removes a chunk of data from the queue. The "full" queue element is swapped On 2015/10/14 14:10:16, kwiberg-webrtc wrote: > "chunk of data" -> "T" Done. https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:145: void CheckQueueInvariance() { On 2015/10/14 14:10:16, kwiberg-webrtc wrote: > invariance -> invariants. Otherwise, it sounds as if the whole queue is > invariant, i.e. constant. > > Also, queue -> element, since that's what it checks. > > End result: CheckElementInvariants. I think that what we do is to verify the compliance of the elements to ItemVerifier, which actually not necessarily should mean that the elements are invariants. I changed it to reflect that. Please behold and comment! https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... File webrtc/common_audio/swap_queue_unittest.cc (right): https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue_unittest.cc:27: // Provides thread_safe random numbers. On 2015/10/14 14:10:16, kwiberg-webrtc wrote: > The comment says nothing that the class name doesn't already say. Good point! I removed the comment. https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue_unittest.cc:27: // Provides thread_safe random numbers. On 2015/10/14 14:10:16, kwiberg-webrtc wrote: > The comment says nothing that the class name doesn't already say. Done. https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue_unittest.cc:418: EXPECT_DEATH(queue.Insert(&invalid_chunk), ""); On 2015/10/14 14:10:16, kwiberg-webrtc wrote: > Unless your employee ID is 007, you don't get to live twice. :-) https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue_unittest.cc:418: EXPECT_DEATH(queue.Insert(&invalid_chunk), ""); On 2015/10/14 14:10:16, kwiberg-webrtc wrote: > Unless your employee ID is 007, you don't get to live twice. Done.
https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... File webrtc/common_audio/swap_queue_unittest.cc (right): https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue_unittest.cc:35: random_numbers_[k] = rand(); On 2015/10/15 07:34:59, peah-webrtc wrote: > On 2015/10/14 13:05:21, the sun wrote: > > Note that it doesn't matter that we make rand() thread safe here. Some other > > thread could be using it anyway. > > > > There's a simple PRNG class in the BWE test framework that we could possibly > > move into webrtc/test/ and reuse. It doesn't even need a lock, since then each > > thread could have its own instance. > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... > > You are fully correct! If ok with you, I'll keep the functor implementation and > use rand in that. That removes the warning at least. Not ok. It still has global mutable state, implicitly, via the rand() implementation. Tests should be as deterministic as possible. If you're running multiple threads using rand(), you cannot make sure that the sequence of random numbers for each thread is the same for successive test runs. Your initial class tried to solve this by pre-generating a shared array of N PRNs, but fell short on thread safety when accessing the shared user_indices_ map. I'd really rather like it if we had a simple PRNG class that can be instantiated on each thread. https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue_unittest.cc:69: }; On 2015/10/14 12:16:48, peah-webrtc wrote: > On 2015/10/13 13:54:24, kwiberg-webrtc wrote: > > Wouldn't it be simpler to just protect rand() calls with a lock? > > Fully true! This is definitely over-engineered! I rewrote it using a functor > instead. See above comment. You will still have non-determinism, just with a little less probability of pulling the same number from the PRNG hat in several threads simultaneously. https://codereview.webrtc.org/1398473004/diff/80001/webrtc/common_audio/swap_... File webrtc/common_audio/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/80001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:92: // optional invariance verifier callback function that verifies Didn't you just say the verifier function doesn't check the invariant? :P Suggest this line is changed to: // optional callback function that verifies https://codereview.webrtc.org/1398473004/diff/80001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:95: bool (*ItemVerifier)(const T&) = I like ItemVerifier https://codereview.webrtc.org/1398473004/diff/80001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:176: void VerifyQueueContent() { For extra niceness, if we let this function return a bool (always return true), then the call sites could be: RTC_DCHECK(VerifyQueueContent()); ...and we'd be sure the code is dropped for non-debug builds.
https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... File webrtc/common_audio/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:60: // Default item invariance verifier callback function. On 2015/10/15 07:35:00, peah-webrtc wrote: > On 2015/10/14 13:05:21, the sun wrote: > > Please use the term "invariant" as that is more common in programming than > > invariance (which is more common in the world of signals...) > > https://www.google.se/search?q=invariant+definition&oq=invariant > > > > also, prepend "SwapQueue" since this is in the vast webrtc:: namespace. > > I'm still not fully happy with invariant. I change it to verifier, does that > sound ok? That works too. But the usual meaning of "invariant" in the context of data structures is simply "predicate that always holds", so checking an invariant is exactly what the supplied function does. https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:71: DefaultItemInvarianceVerifier> On 2015/10/15 07:35:00, peah-webrtc wrote: > On 2015/10/14 14:10:16, kwiberg-webrtc wrote: > > Not too happy with this name. Maybe simply "ItemChecker"? > > Me neither. I changed it now to ItemVerifier. In my mind, the function should > verify the properties of the items. ItemChecker is close to that but I think it > is a bit more vague. CheckItemInvariant I think is a bit narrow since there is > actually not any strict requirement that the items should be invariant. > > Any opinions or further suggestions? That we allow the user to require an invariant (which is just a predicate) to hold is no restriction. The user is free to specify as lax a predicate as she likes. That said, "verifier" works too. https://codereview.webrtc.org/1398473004/diff/80001/webrtc/common_audio/swap_... File webrtc/common_audio/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/80001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:80: // found in the unit tests. This text would fit better immediately preceding the SwapQueue class definition. https://codereview.webrtc.org/1398473004/diff/80001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:95: bool (*ItemVerifier)(const T&) = On 2015/10/15 08:44:01, the sun wrote: > I like ItemVerifier +1 https://codereview.webrtc.org/1398473004/diff/80001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:176: void VerifyQueueContent() { On 2015/10/15 08:44:00, the sun wrote: > For extra niceness, if we let this function return a bool (always return true), > then the call sites could be: > > RTC_DCHECK(VerifyQueueContent()); > > ...and we'd be sure the code is dropped for non-debug builds. Yes, good idea. And as an added bonus, the line number reported by a failing check will be more specific.
https://codereview.webrtc.org/1398473004/diff/80001/webrtc/common_audio/swap_... File webrtc/common_audio/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/80001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:80: // found in the unit tests. On 2015/10/15 08:55:40, kwiberg-webrtc wrote: > This text would fit better immediately preceding the SwapQueue class definition. Also, Fredrik and I worked out an alternative text that's more example- than math-oriented. The full class description reads: This class is a fixed-size queue. A producer calls Insert() to insert an element of type T at the back of the queue, and a consumer calls Remove() to remove an element from the front of the queue. It's safe for the producer(s) and the consumer(s) to access the queue concurrently, from different threads. To avoid the construction, copying, and destruction of Ts that a naive queue implementation would require, for each "full" T passed from producer to consumer, SwapQueue<T> passes an "empty" T in the other direction (an "empty" T is one that contains nothing of value for the consumer). This bidirectional movement is implemented with swap(). // Create queue: Bottle proto(568); // Prepare an empty Bottle. Heap allocates space for 568 ml. SwapQueue<Bottle> q(N, proto); // Init queue with N copies of proto. Also heap. // Producer pseudo-code: Bottle b(568); // Prepare an empty Bottle. Heap allocates space for 568 ml. loop { b.Fill(amount); // Where amount <= 568 ml. q.Insert(&b); // Swap our full Bottle for an empty one from q. } // Consumer pseudo-code: Bottle b(568); // Prepare an empty Bottle. Heap allocates space for 568 ml. loop { q.Remove(&b); // Swap our empty Bottle for the next-in-line full Bottle. Drink(&b); } For a well-behaved Bottle class, there are no allocations in the producer, since it just fills an empty Bottle that's already large enough; no deallocations in the consumer, since it returns each empty Bottle to the queue after having drunk it; and no copies along the way, since the queue uses swap() everywhere to move full Bottles in one direction and empty ones in the other.
https://codereview.webrtc.org/1398473004/diff/80001/webrtc/common_audio/swap_... File webrtc/common_audio/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/80001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:132: using std::swap; Maybe a comment here why using std::swap is above actual swap usage, and example of overloaded swap?
https://codereview.webrtc.org/1398473004/diff/80001/webrtc/common_audio/swap_... File webrtc/common_audio/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/80001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:132: using std::swap; On 2015/10/19 10:36:12, the sun wrote: > Maybe a comment here why using std::swap is above actual swap usage, and example > of overloaded swap? Hmm. Difficult to find a comment that's unobtrusive enough, given that this is the standard idiom for swapping an arbitrary type. Maybe using std::swap; // Fall back to std::swap if no swap() if found with ADL. swap(...); but personally, I'm inclined to just keep silent and let each reader go through the process of figuring this out. It doesn't take that much googling, and you only have to do it once.
On 2015/10/19 10:47:17, kwiberg-webrtc wrote: > https://codereview.webrtc.org/1398473004/diff/80001/webrtc/common_audio/swap_... > File webrtc/common_audio/swap_queue.h (right): > > https://codereview.webrtc.org/1398473004/diff/80001/webrtc/common_audio/swap_... > webrtc/common_audio/swap_queue.h:132: using std::swap; > On 2015/10/19 10:36:12, the sun wrote: > > Maybe a comment here why using std::swap is above actual swap usage, and > example > > of overloaded swap? > > Hmm. Difficult to find a comment that's unobtrusive enough, given that this is > the standard idiom for swapping an arbitrary type. Maybe > > using std::swap; // Fall back to std::swap if no swap() if found with ADL. > swap(...); > > but personally, I'm inclined to just keep silent and let each reader go through > the process of figuring this out. It doesn't take that much googling, and you > only have to do it once. // Permits overloading swap() with own implementation, see example in (some_test.cc), but permitting fallback to std::swap.
On 2015/10/19 11:13:36, pbos-webrtc wrote: > On 2015/10/19 10:47:17, kwiberg-webrtc wrote: > > > https://codereview.webrtc.org/1398473004/diff/80001/webrtc/common_audio/swap_... > > File webrtc/common_audio/swap_queue.h (right): > > > > > https://codereview.webrtc.org/1398473004/diff/80001/webrtc/common_audio/swap_... > > webrtc/common_audio/swap_queue.h:132: using std::swap; > > On 2015/10/19 10:36:12, the sun wrote: > > > Maybe a comment here why using std::swap is above actual swap usage, and > > example > > > of overloaded swap? > > > > Hmm. Difficult to find a comment that's unobtrusive enough, given that this is > > the standard idiom for swapping an arbitrary type. Maybe > > > > using std::swap; // Fall back to std::swap if no swap() if found with ADL. > > swap(...); > > > > but personally, I'm inclined to just keep silent and let each reader go > through > > the process of figuring this out. It doesn't take that much googling, and you > > only have to do it once. > > // Permits overloading swap() with own implementation, see example in > (some_test.cc), but permitting fallback to std::swap. That's pretty much what I said, except it doesn't fit on one line... If my one-liner is too cryptic, then maybe // Fall back to std::swap if no type-specific swap() is found. // (See e.g. http://en.cppreference.com/w/cpp/concept/Swappable.) using std::swap; swap(...); but I really wouldn't like to see each instance of this idiom encrusted with *two* lines of extra comments. One already felt like too much.
solenberg@webrtc.org changed reviewers: - solenberg@google.com
Description was changed from ========== Changed queue implementation to the proposed vector-based solution. Added unit tests. BUG= ========== to ========== Changed queue implementation to the proposed vector-based solution. Added unit tests. BUG=webrtc:5099 ==========
https://codereview.webrtc.org/1398473004/diff/80001/webrtc/common_audio/swap_... File webrtc/common_audio/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/80001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:80: // found in the unit tests. On 2015/10/15 12:47:37, kwiberg-webrtc wrote: > On 2015/10/15 08:55:40, kwiberg-webrtc wrote: > > This text would fit better immediately preceding the SwapQueue class > definition. > > Also, Fredrik and I worked out an alternative text that's more example- than > math-oriented. The full class description reads: > > This class is a fixed-size queue. A producer calls Insert() to insert > an element of type T at the back of the queue, and a consumer calls > Remove() to remove an element from the front of the queue. It's safe > for the producer(s) and the consumer(s) to access the queue > concurrently, from different threads. > > To avoid the construction, copying, and destruction of Ts that a naive > queue implementation would require, for each "full" T passed from > producer to consumer, SwapQueue<T> passes an "empty" T in the other > direction (an "empty" T is one that contains nothing of value for the > consumer). This bidirectional movement is implemented with swap(). > > // Create queue: > Bottle proto(568); // Prepare an empty Bottle. Heap allocates space for 568 ml. > SwapQueue<Bottle> q(N, proto); // Init queue with N copies of proto. Also heap. > > // Producer pseudo-code: > Bottle b(568); // Prepare an empty Bottle. Heap allocates space for 568 ml. > loop { > b.Fill(amount); // Where amount <= 568 ml. > q.Insert(&b); // Swap our full Bottle for an empty one from q. > } > > // Consumer pseudo-code: > Bottle b(568); // Prepare an empty Bottle. Heap allocates space for 568 ml. > loop { > q.Remove(&b); // Swap our empty Bottle for the next-in-line full Bottle. > Drink(&b); > } > > For a well-behaved Bottle class, there are no allocations in the > producer, since it just fills an empty Bottle that's already large > enough; no deallocations in the consumer, since it returns each empty > Bottle to the queue after having drunk it; and no copies along the > way, since the queue uses swap() everywhere to move full Bottles in > one direction and empty ones in the other. I like that :-) Will change! https://codereview.webrtc.org/1398473004/diff/80001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:92: // optional invariance verifier callback function that verifies On 2015/10/15 08:44:00, the sun wrote: > Didn't you just say the verifier function doesn't check the invariant? :P > > Suggest this line is changed to: > // optional callback function that verifies Good find! Will fix! https://codereview.webrtc.org/1398473004/diff/80001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:92: // optional invariance verifier callback function that verifies On 2015/10/15 08:44:00, the sun wrote: > Didn't you just say the verifier function doesn't check the invariant? :P > > Suggest this line is changed to: > // optional callback function that verifies Done. https://codereview.webrtc.org/1398473004/diff/80001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:95: bool (*ItemVerifier)(const T&) = On 2015/10/15 08:44:01, the sun wrote: > I like ItemVerifier Acknowledged. https://codereview.webrtc.org/1398473004/diff/80001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:95: bool (*ItemVerifier)(const T&) = On 2015/10/15 08:55:40, kwiberg-webrtc wrote: > On 2015/10/15 08:44:01, the sun wrote: > > I like ItemVerifier > > +1 Acknowledged. https://codereview.webrtc.org/1398473004/diff/80001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:132: using std::swap; On 2015/10/19 10:47:17, kwiberg-webrtc wrote: > On 2015/10/19 10:36:12, the sun wrote: > > Maybe a comment here why using std::swap is above actual swap usage, and > example > > of overloaded swap? > > Hmm. Difficult to find a comment that's unobtrusive enough, given that this is > the standard idiom for swapping an arbitrary type. Maybe > > using std::swap; // Fall back to std::swap if no swap() if found with ADL. > swap(...); > > but personally, I'm inclined to just keep silent and let each reader go through > the process of figuring this out. It doesn't take that much googling, and you > only have to do it once. I'd also prefer to keep it silent, since it is a standard idiom. https://codereview.webrtc.org/1398473004/diff/80001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:176: void VerifyQueueContent() { On 2015/10/15 08:44:00, the sun wrote: > For extra niceness, if we let this function return a bool (always return true), > then the call sites could be: > > RTC_DCHECK(VerifyQueueContent()); > > ...and we'd be sure the code is dropped for non-debug builds. Awesome! https://codereview.webrtc.org/1398473004/diff/80001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:176: void VerifyQueueContent() { On 2015/10/15 08:44:00, the sun wrote: > For extra niceness, if we let this function return a bool (always return true), > then the call sites could be: > > RTC_DCHECK(VerifyQueueContent()); > > ...and we'd be sure the code is dropped for non-debug builds. Done. https://codereview.webrtc.org/1398473004/diff/80001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:176: void VerifyQueueContent() { On 2015/10/15 08:55:40, kwiberg-webrtc wrote: > On 2015/10/15 08:44:00, the sun wrote: > > For extra niceness, if we let this function return a bool (always return > true), > > then the call sites could be: > > > > RTC_DCHECK(VerifyQueueContent()); > > > > ...and we'd be sure the code is dropped for non-debug builds. > > Yes, good idea. And as an added bonus, the line number reported by a failing > check will be more specific. Acknowledged.
https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/BUILD.gn (right): https://codereview.webrtc.org/1398473004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/BUILD.gn:91: "swapped_nonblocking_queue.h", On 2015/10/13 12:34:26, the sun wrote: > On 2015/10/13 12:09:07, kwiberg-webrtc wrote: > > On 2015/10/13 11:56:11, peah-webrtc wrote: > > > On 2015/10/13 06:37:57, hlundin-webrtc wrote: > > > > I wonder if this should be sitting in common_audio instead, together with > > the > > > > RingBuffer and other friends. > > > > > > I think that makes sense! I will move it, but if anyone objects, please let > me > > > know and we can discuss it further. > > > > It might be generic enough to merit a place in webrtc/base, since it's not > > audio-specific. > > Yes, in time. I think first we let it get some mileage within our little world. Acknowledged. https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:25: // It is safe to access the class concurrently from multiple threads. On 2015/10/13 12:34:27, the sun wrote: > On 2015/10/13 11:56:13, peah-webrtc wrote: > > On 2015/10/13 07:53:05, solenberg wrote: > > > Will git cl format fix the alignment of comment lines to 80 chars? > > > > You mean whether multiline comments are rearranged to fit better on 80 chars? > > yes Acknowledged. https://codereview.webrtc.org/1398473004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/swap_queue.h:36: class SwapQueue { On 2015/10/13 12:09:07, kwiberg-webrtc wrote: > On 2015/10/13 11:56:12, peah-webrtc wrote: > > On 2015/10/13 07:53:05, solenberg wrote: > > > On 2015/10/12 14:05:48, kwiberg-webrtc wrote: > > > > It might be a good idea to reflect the threadsafeness in the class name, > > since > > > > the usual case is for simple data structures not to be threadsafe. > > > > ThreadsafeSwapQueue? > > > > > > +1 -1 > > > > > > I think that is a good idea assuming we are moving towards a world where > > nothing > > > is thread safe except for things that begin with "Threadsafe". > > > I think it is a bad idea to have that annotation in the name since we likely > > > will never reach that point, but still can assume nothing about thread > > > safeness... > > > > How about omitting threadsafety in the name, but opening up for this to be > > explicitly turned on/off in the constructor and having two internal > code-paths, > > one threadsafe and one which is not? > > Dynamically taking locks or not would make the code more complicated than > necessary. I'd suggest locking unconditionally like you do now, or possibly > passing the mutex type as a second (optional) template parameter. But... no, > just keep the current code until you see a need for a non-threadsafe variant. Acknowledged. https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... File webrtc/common_audio/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:74: // } On 2015/10/14 14:10:16, kwiberg-webrtc wrote: > On 2015/10/14 12:16:47, peah-webrtc wrote: > > On 2015/10/13 13:54:24, kwiberg-webrtc wrote: > > > This example is probably a bit too long to clearly illustrate the point, and > > > doesn't reflect the intended real-world usage. It might be better to make a > > real > > > unit test with a toy producer and a toy consumer, and have EXPECTs on the > > > vectors' .capacity() that ensure that no run-time reallocation of vectors > > takes > > > place. > > > > I have that as well, should I just refer to that instead and remove this > > example? > > Yes, provided it's small and clean enough to work as an illustrative example. Acknowledged. https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... File webrtc/common_audio/swap_queue_unittest.cc (right): https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue_unittest.cc:35: random_numbers_[k] = rand(); On 2015/10/15 08:44:00, the sun wrote: > On 2015/10/15 07:34:59, peah-webrtc wrote: > > On 2015/10/14 13:05:21, the sun wrote: > > > Note that it doesn't matter that we make rand() thread safe here. Some other > > > thread could be using it anyway. > > > > > > There's a simple PRNG class in the BWE test framework that we could possibly > > > move into webrtc/test/ and reuse. It doesn't even need a lock, since then > each > > > thread could have its own instance. > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... > > > > You are fully correct! If ok with you, I'll keep the functor implementation > and > > use rand in that. That removes the warning at least. > > Not ok. It still has global mutable state, implicitly, via the rand() > implementation. > > Tests should be as deterministic as possible. If you're running multiple threads > using rand(), you cannot make sure that the sequence of random numbers for each > thread is the same for successive test runs. Your initial class tried to solve > this by pre-generating a shared array of N PRNs, but fell short on thread safety > when accessing the shared user_indices_ map. > > I'd really rather like it if we had a simple PRNG class that can be instantiated > on each thread. I definitely agree on the issues with non-determinism in tests and I think that in this case the lock test is not at all motivated as the queue lock is so simple. I'll remove it. https://codereview.webrtc.org/1398473004/diff/40001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue_unittest.cc:69: }; On 2015/10/15 08:44:00, the sun wrote: > On 2015/10/14 12:16:48, peah-webrtc wrote: > > On 2015/10/13 13:54:24, kwiberg-webrtc wrote: > > > Wouldn't it be simpler to just protect rand() calls with a lock? > > > > Fully true! This is definitely over-engineered! I rewrote it using a functor > > instead. > > See above comment. You will still have non-determinism, just with a little less > probability of pulling the same number from the PRNG hat in several threads > simultaneously. Acknowledged. https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... File webrtc/common_audio/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:60: // Default item invariance verifier callback function. On 2015/10/15 08:55:40, kwiberg-webrtc wrote: > On 2015/10/15 07:35:00, peah-webrtc wrote: > > On 2015/10/14 13:05:21, the sun wrote: > > > Please use the term "invariant" as that is more common in programming than > > > invariance (which is more common in the world of signals...) > > > https://www.google.se/search?q=invariant+definition&oq=invariant > > > > > > also, prepend "SwapQueue" since this is in the vast webrtc:: namespace. > > > > I'm still not fully happy with invariant. I change it to verifier, does that > > sound ok? > > That works too. But the usual meaning of "invariant" in the context of data > structures is simply "predicate that always holds", so checking an invariant is > exactly what the supplied function does. Not sure about the conclusion, should we go for invariant or verifier? The new functor implementation makes things a bit different as the verifier does no longer need to be invariant over time. https://codereview.webrtc.org/1398473004/diff/60001/webrtc/common_audio/swap_... webrtc/common_audio/swap_queue.h:71: DefaultItemInvarianceVerifier> On 2015/10/15 08:55:40, kwiberg-webrtc wrote: > On 2015/10/15 07:35:00, peah-webrtc wrote: > > On 2015/10/14 14:10:16, kwiberg-webrtc wrote: > > > Not too happy with this name. Maybe simply "ItemChecker"? > > > > Me neither. I changed it now to ItemVerifier. In my mind, the function should > > verify the properties of the items. ItemChecker is close to that but I think > it > > is a bit more vague. CheckItemInvariant I think is a bit narrow since there is > > actually not any strict requirement that the items should be invariant. > > > > Any opinions or further suggestions? > > That we allow the user to require an invariant (which is just a predicate) to > hold is no restriction. The user is free to specify as lax a predicate as she > likes. That said, "verifier" works too. Acknowledged.
https://codereview.webrtc.org/1398473004/diff/120001/webrtc/common_audio/swap... File webrtc/common_audio/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/120001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:26: // from the other end. The queue can be applied to any swappable objects. objects -> type https://codereview.webrtc.org/1398473004/diff/120001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:27: // The queue is safe to access the class concurrently from multiple threads. Remove "the class" https://codereview.webrtc.org/1398473004/diff/120001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:29: // Conceptually, the queue maintains an ordered sequence and an unordered bunch Where's the shorter description that me and Karl suggested? https://codereview.webrtc.org/1398473004/diff/120001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:101: // Queue template implementing a queue of elements of type T and with an Too verbose. "Queue on type T." https://codereview.webrtc.org/1398473004/diff/120001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:104: template <typename T, typename QueueItemVerifier = SwapQueueItemVerifier<T> > QueueItemVerifier shouldn't be needed as a template argument now that you can give it to the c-tor. https://codereview.webrtc.org/1398473004/diff/120001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:116: explicit SwapQueue(size_t size, const QueueItemVerifier& queue_item_verifier) remove "explicit" https://codereview.webrtc.org/1398473004/diff/120001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:208: remove blank https://codereview.webrtc.org/1398473004/diff/120001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:219: // 0 <= next_write_index_ <= queue.size() next_write_index_ < queue.size() https://codereview.webrtc.org/1398473004/diff/120001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:221: // 0 <= next_read_index_ <= queue.size() next_read_index_ < queue.size() https://codereview.webrtc.org/1398473004/diff/120001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:227: rtc::CriticalSection crit_queue_; nit: I would have used the order: queue_item_verifier_ crit_queue_; indices queue_ crit_queue_ becomes a header for the attributes it protects https://codereview.webrtc.org/1398473004/diff/120001/webrtc/common_audio/swap... File webrtc/common_audio/swap_queue_unittest.cc (right): https://codereview.webrtc.org/1398473004/diff/120001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:27: class ThreadSafeRandomNumberGenerator { Not used, remove!
https://codereview.webrtc.org/1398473004/diff/120001/webrtc/common_audio/swap... File webrtc/common_audio/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/120001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:26: // from the other end. The queue can be applied to any swappable objects. On 2015/10/26 15:16:20, the sun wrote: > objects -> type Please see below. https://codereview.webrtc.org/1398473004/diff/120001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:27: // The queue is safe to access the class concurrently from multiple threads. On 2015/10/26 15:16:20, the sun wrote: > Remove "the class" Please see below. https://codereview.webrtc.org/1398473004/diff/120001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:29: // Conceptually, the queue maintains an ordered sequence and an unordered bunch On 2015/10/26 15:16:21, the sun wrote: > Where's the shorter description that me and Karl suggested? That is a very good question. Seems like something went wrong on the way. Will add it again and update. Sorry about that! https://codereview.webrtc.org/1398473004/diff/120001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:101: // Queue template implementing a queue of elements of type T and with an On 2015/10/26 15:16:21, the sun wrote: > Too verbose. > "Queue on type T." Done. https://codereview.webrtc.org/1398473004/diff/120001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:104: template <typename T, typename QueueItemVerifier = SwapQueueItemVerifier<T> > On 2015/10/26 15:16:21, the sun wrote: > QueueItemVerifier shouldn't be needed as a template argument now that you can > give it to the c-tor. How do you mean? I still need to have the type of QueueItemVerifier to be able to store it in a type-safe manner inside the Queue, right? Not sure on how to do that without the type as a template argument. https://codereview.webrtc.org/1398473004/diff/120001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:116: explicit SwapQueue(size_t size, const QueueItemVerifier& queue_item_verifier) On 2015/10/26 15:16:20, the sun wrote: > remove "explicit" Done. https://codereview.webrtc.org/1398473004/diff/120001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:208: On 2015/10/26 15:16:20, the sun wrote: > remove blank Done. https://codereview.webrtc.org/1398473004/diff/120001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:219: // 0 <= next_write_index_ <= queue.size() On 2015/10/26 15:16:21, the sun wrote: > next_write_index_ < queue.size() Done. https://codereview.webrtc.org/1398473004/diff/120001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:221: // 0 <= next_read_index_ <= queue.size() On 2015/10/26 15:16:21, the sun wrote: > next_read_index_ < queue.size() Done. https://codereview.webrtc.org/1398473004/diff/120001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:227: rtc::CriticalSection crit_queue_; On 2015/10/26 15:16:21, the sun wrote: > nit: I would have used the order: > > queue_item_verifier_ > > crit_queue_; > indices > queue_ > > crit_queue_ becomes a header for the attributes it protects Makes sense! Will change to that. https://codereview.webrtc.org/1398473004/diff/120001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:227: rtc::CriticalSection crit_queue_; On 2015/10/26 15:16:21, the sun wrote: > nit: I would have used the order: > > queue_item_verifier_ > > crit_queue_; > indices > queue_ > > crit_queue_ becomes a header for the attributes it protects Done. https://codereview.webrtc.org/1398473004/diff/120001/webrtc/common_audio/swap... File webrtc/common_audio/swap_queue_unittest.cc (right): https://codereview.webrtc.org/1398473004/diff/120001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:27: class ThreadSafeRandomNumberGenerator { On 2015/10/26 15:16:21, the sun wrote: > Not used, remove! Done.
https://codereview.webrtc.org/1398473004/diff/140001/webrtc/common_audio/swap... File webrtc/common_audio/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/140001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:41: // // Also heap. Since it doesn't fit on one line anyway: "Also heap." -> "Each copy allocates on the heap." https://codereview.webrtc.org/1398473004/diff/140001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:61: // one direction and empty ones in the other. Move lines 25-61 to just before the SwapQueue class declaration? And put // on the two empty lines, so that it all hangs together? https://codereview.webrtc.org/1398473004/diff/140001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:66: bool SwapQueueItemVerifierFunction(const T&) { Prepend "Default" or "Noop" or something to this name, so that it doesn't sound like a generic verifier? Also, it's a bit on the short side as is. https://codereview.webrtc.org/1398473004/diff/140001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:84: template <typename T, typename QueueItemVerifier = SwapQueueItemVerifier<T> > "> >" -> ">>" https://codereview.webrtc.org/1398473004/diff/140001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:115: // Resets the queue to have zero content. Point out that the number of slots in the queue doesn't change? https://codereview.webrtc.org/1398473004/diff/140001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:124: // the queue (an object from the unordered bunch). The doc now talks about "empty" and "full" Ts again, so maybe Inserts a "full" T at the back of the queue by swapping *input with an "empty" T from the queue. https://codereview.webrtc.org/1398473004/diff/140001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:129: bool Insert(T* input) { Annotate this with WARN_UNUSED_RESULT? https://codereview.webrtc.org/1398473004/diff/140001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:142: next_write_index_++; In general, prefer the prefix forms. (They're sometimes more efficient, so it's a good habit to get into, and it makes the code more consistent.) https://codereview.webrtc.org/1398473004/diff/140001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:154: // unordered bunch in the queue). See comment above. Maybe Removes the frontmost "full" T from the queue by swapping it with the "empty" T *output. https://codereview.webrtc.org/1398473004/diff/140001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:159: bool Remove(T* output) { WARN_UNUSED_RESULT? https://codereview.webrtc.org/1398473004/diff/140001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:183: bool VerifyQueueContent() { I'd say it ought to be "Contents". See e.g. http://dictionary.cambridge.org/grammar/british-grammar/content-or-contents. https://codereview.webrtc.org/1398473004/diff/140001/webrtc/common_audio/swap... File webrtc/common_audio/swap_queue_unittest.cc (right): https://codereview.webrtc.org/1398473004/diff/140001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:44: return ((id_ == m.id_) && (info_ == m.info_)); Cut down on the number of parentheses? https://codereview.webrtc.org/1398473004/diff/140001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:88: EXPECT_DEATH(queue.Insert(&invalid_value), ""); Protect this with #ifdefs, since some of our platforms are pro-life and won't allow death tests? https://codereview.webrtc.org/1398473004/diff/140001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:139: } Initialize the vector empty, and use push_back to insert messages?
https://codereview.webrtc.org/1398473004/diff/120001/webrtc/common_audio/swap... File webrtc/common_audio/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/120001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:104: template <typename T, typename QueueItemVerifier = SwapQueueItemVerifier<T> > On 2015/10/26 22:27:02, peah-webrtc wrote: > On 2015/10/26 15:16:21, the sun wrote: > > QueueItemVerifier shouldn't be needed as a template argument now that you can > > give it to the c-tor. > > How do you mean? I still need to have the type of QueueItemVerifier to be able > to store it in a type-safe manner inside the Queue, right? Not sure on how to do > that without the type as a template argument. You're right. The template argument is needed.
https://codereview.webrtc.org/1398473004/diff/140001/webrtc/common_audio/swap... File webrtc/common_audio/swap_queue_unittest.cc (right): https://codereview.webrtc.org/1398473004/diff/140001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:62: bool InvarianceVerifierFunction(const std::vector<int>& v) { You call this ItemVerifier elsewhere. Can we stick to one name please - preferably just "ItemVerifier". https://codereview.webrtc.org/1398473004/diff/140001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:79: AFAICT you don't test instantiating the queue using all available c-tors. The test don't need to be exhaustive, but make sure each c-tor is used once.
https://codereview.webrtc.org/1398473004/diff/120001/webrtc/common_audio/swap... File webrtc/common_audio/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/120001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:104: template <typename T, typename QueueItemVerifier = SwapQueueItemVerifier<T> > On 2015/10/27 13:51:33, the sun wrote: > On 2015/10/26 22:27:02, peah-webrtc wrote: > > On 2015/10/26 15:16:21, the sun wrote: > > > QueueItemVerifier shouldn't be needed as a template argument now that you > can > > > give it to the c-tor. > > > > How do you mean? I still need to have the type of QueueItemVerifier to be able > > to store it in a type-safe manner inside the Queue, right? Not sure on how to > do > > that without the type as a template argument. > > You're right. The template argument is needed. Acknowledged. https://codereview.webrtc.org/1398473004/diff/140001/webrtc/common_audio/swap... File webrtc/common_audio/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/140001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:41: // // Also heap. On 2015/10/27 12:05:13, kwiberg-webrtc wrote: > Since it doesn't fit on one line anyway: > > "Also heap." -> "Each copy allocates on the heap." Done. https://codereview.webrtc.org/1398473004/diff/140001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:61: // one direction and empty ones in the other. On 2015/10/27 12:05:13, kwiberg-webrtc wrote: > Move lines 25-61 to just before the SwapQueue class declaration? And put // on > the two empty lines, so that it all hangs together? Done. https://codereview.webrtc.org/1398473004/diff/140001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:66: bool SwapQueueItemVerifierFunction(const T&) { On 2015/10/27 12:05:13, kwiberg-webrtc wrote: > Prepend "Default" or "Noop" or something to this name, so that it doesn't sound > like a generic verifier? Also, it's a bit on the short side as is. Done. https://codereview.webrtc.org/1398473004/diff/140001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:84: template <typename T, typename QueueItemVerifier = SwapQueueItemVerifier<T> > On 2015/10/27 12:05:13, kwiberg-webrtc wrote: > "> >" -> ">>" Done. https://codereview.webrtc.org/1398473004/diff/140001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:115: // Resets the queue to have zero content. On 2015/10/27 12:05:13, kwiberg-webrtc wrote: > Point out that the number of slots in the queue doesn't change? Done. https://codereview.webrtc.org/1398473004/diff/140001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:124: // the queue (an object from the unordered bunch). On 2015/10/27 12:05:13, kwiberg-webrtc wrote: > The doc now talks about "empty" and "full" Ts again, so maybe > > Inserts a "full" T at the back of the queue by swapping *input with an "empty" > T from the queue. Done. https://codereview.webrtc.org/1398473004/diff/140001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:129: bool Insert(T* input) { On 2015/10/27 12:05:13, kwiberg-webrtc wrote: > Annotate this with WARN_UNUSED_RESULT? Done. https://codereview.webrtc.org/1398473004/diff/140001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:142: next_write_index_++; On 2015/10/27 12:05:13, kwiberg-webrtc wrote: > In general, prefer the prefix forms. (They're sometimes more efficient, so it's > a good habit to get into, and it makes the code more consistent.) Thanks! Will do! Done. https://codereview.webrtc.org/1398473004/diff/140001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:154: // unordered bunch in the queue). On 2015/10/27 12:05:13, kwiberg-webrtc wrote: > See comment above. Maybe > > Removes the frontmost "full" T from the queue by swapping it with the "empty" > T *output. Done. https://codereview.webrtc.org/1398473004/diff/140001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:159: bool Remove(T* output) { On 2015/10/27 12:05:13, kwiberg-webrtc wrote: > WARN_UNUSED_RESULT? Done. https://codereview.webrtc.org/1398473004/diff/140001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:183: bool VerifyQueueContent() { On 2015/10/27 12:05:13, kwiberg-webrtc wrote: > I'd say it ought to be "Contents". See e.g. > http://dictionary.cambridge.org/grammar/british-grammar/content-or-contents. Done. https://codereview.webrtc.org/1398473004/diff/140001/webrtc/common_audio/swap... File webrtc/common_audio/swap_queue_unittest.cc (right): https://codereview.webrtc.org/1398473004/diff/140001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:44: return ((id_ == m.id_) && (info_ == m.info_)); On 2015/10/27 12:05:13, kwiberg-webrtc wrote: > Cut down on the number of parentheses? Done. https://codereview.webrtc.org/1398473004/diff/140001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:62: bool InvarianceVerifierFunction(const std::vector<int>& v) { On 2015/10/27 16:18:48, the sun wrote: > You call this ItemVerifier elsewhere. Can we stick to one name please - > preferably just "ItemVerifier". Done. https://codereview.webrtc.org/1398473004/diff/140001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:79: On 2015/10/27 16:18:48, the sun wrote: > AFAICT you don't test instantiating the queue using all available c-tors. The > test don't need to be exhaustive, but make sure each c-tor is used once. Good point! And found an error that indeed did :-). Done. https://codereview.webrtc.org/1398473004/diff/140001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:88: EXPECT_DEATH(queue.Insert(&invalid_value), ""); On 2015/10/27 12:05:13, kwiberg-webrtc wrote: > Protect this with #ifdefs, since some of our platforms are pro-life and won't > allow death tests? Done. https://codereview.webrtc.org/1398473004/diff/140001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:139: } On 2015/10/27 12:05:13, kwiberg-webrtc wrote: > Initialize the vector empty, and use push_back to insert messages? Done.
https://codereview.webrtc.org/1398473004/diff/160001/webrtc/common_audio/swap... File webrtc/common_audio/swap_queue_unittest.cc (right): https://codereview.webrtc.org/1398473004/diff/160001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:17: #include "webrtc/common_audio/swap_queue.h" Include swap_queue.h first, since we don't have any .cc file for it. http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Names_and_Ord... https://codereview.webrtc.org/1398473004/diff/160001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:20: #include "webrtc/system_wrappers/interface/sleep.h" Clean away unused includes, please. https://codereview.webrtc.org/1398473004/diff/160001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:28: class SwapQueueTestMessage { nit: just TestMessage using the SwapQueue prefix makes it sound like it is actually tied to the SwapQueue class, like SwapQueueItemVerifier https://codereview.webrtc.org/1398473004/diff/160001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:43: bool Compare(const SwapQueueTestMessage& m) { Total nit: This is C++, we can do "bool operator==(...)" https://codereview.webrtc.org/1398473004/diff/160001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:62: RTC_CHECK(v.size() == kChunkSize); Shouldn't use RTC_CHECK here. The verifier should just return true/false and SwapQueue will act on the result. https://codereview.webrtc.org/1398473004/diff/160001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:92: TEST(SwapQueueTest, SuccessfulInvarianceVerification1) { Avoid numbering the tests when you could let the name have some meaning: "SuccessfulItemVerifyFunction". ItemVerify, because ItemVerifier is the name we decided to use. https://codereview.webrtc.org/1398473004/diff/160001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:92: TEST(SwapQueueTest, SuccessfulInvarianceVerification1) { nit: could you please rearrange the tests to start with bread-and-butter stuff (construct/destruct, insert, remove, full/empty, ...). https://codereview.webrtc.org/1398473004/diff/160001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:105: TEST(SwapQueueTest, SuccessfulInvarianceVerification2) { nit: for consistency, "SuccessfulItemVerifyFunctor" https://codereview.webrtc.org/1398473004/diff/160001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:118: #if defined(GTEST_HAS_DEATH_TEST) Karl use a different condition to enable death tests: https://codereview.webrtc.org/1408403002/diff/240001/webrtc/base/array_view_u... Not sure what is actually required. Can you guys figure out the correct way? https://codereview.webrtc.org/1398473004/diff/160001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:119: TEST(SwapQueueTest, FunctorVerification) { UnsuccessfulItemVerifyFunctor https://codereview.webrtc.org/1398473004/diff/160001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:120: IntVerifier verifier(-2); IntVerifier is only used in this test - can you move the class in here instead? https://codereview.webrtc.org/1398473004/diff/160001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:131: TEST(SwapQueueTest, UnSuccessfulInvarianceVerification1) { UnsuccessfulItemVerifyInsert https://codereview.webrtc.org/1398473004/diff/160001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:141: TEST(SwapQueueTest, UnSuccessfulInvarianceVerification2) { UnsuccessfulItemVerifyRemove https://codereview.webrtc.org/1398473004/diff/160001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:182: std::vector<int16_t> template_queue_element(kFrameLength); Just make this inline, in the queue instantiation: SwapQueue<std::vector<int16_t>> queue(kQueueSize, std::vector<int16_t>(kFrameLength)); https://codereview.webrtc.org/1398473004/diff/160001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:205: TEST(SwapQueueTest, FullQueue) { It would be a good test to verify the Remove()d values for a queue that a) runs full and b) we know will wrap the indices. Either add verification in this test, or extend MessageTest or VectorTest to wrap around. https://codereview.webrtc.org/1398473004/diff/160001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:238: I didn't see a test for Clear().
https://codereview.webrtc.org/1398473004/diff/160001/webrtc/common_audio/swap... File webrtc/common_audio/swap_queue_unittest.cc (right): https://codereview.webrtc.org/1398473004/diff/160001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:17: #include "webrtc/common_audio/swap_queue.h" On 2015/10/29 09:33:04, the sun wrote: > Include swap_queue.h first, since we don't have any .cc file for it. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Names_and_Ord... Done. https://codereview.webrtc.org/1398473004/diff/160001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:20: #include "webrtc/system_wrappers/interface/sleep.h" On 2015/10/29 09:33:04, the sun wrote: > Clean away unused includes, please. Done. https://codereview.webrtc.org/1398473004/diff/160001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:28: class SwapQueueTestMessage { On 2015/10/29 09:33:04, the sun wrote: > nit: just TestMessage > using the SwapQueue prefix makes it sound like it is actually tied to the > SwapQueue class, like SwapQueueItemVerifier Good point! Renamed the class. Done. https://codereview.webrtc.org/1398473004/diff/160001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:43: bool Compare(const SwapQueueTestMessage& m) { On 2015/10/29 09:33:04, the sun wrote: > Total nit: This is C++, we can do "bool operator==(...)" Good point! Done. https://codereview.webrtc.org/1398473004/diff/160001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:62: RTC_CHECK(v.size() == kChunkSize); On 2015/10/29 09:33:04, the sun wrote: > Shouldn't use RTC_CHECK here. The verifier should just return true/false and > SwapQueue will act on the result. Done. https://codereview.webrtc.org/1398473004/diff/160001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:92: TEST(SwapQueueTest, SuccessfulInvarianceVerification1) { On 2015/10/29 09:33:04, the sun wrote: > Avoid numbering the tests when you could let the name have some meaning: > "SuccessfulItemVerifyFunction". > > ItemVerify, because ItemVerifier is the name we decided to use. Done. https://codereview.webrtc.org/1398473004/diff/160001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:92: TEST(SwapQueueTest, SuccessfulInvarianceVerification1) { On 2015/10/29 09:33:04, the sun wrote: > Avoid numbering the tests when you could let the name have some meaning: > "SuccessfulItemVerifyFunction". > > ItemVerify, because ItemVerifier is the name we decided to use. Done. https://codereview.webrtc.org/1398473004/diff/160001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:105: TEST(SwapQueueTest, SuccessfulInvarianceVerification2) { On 2015/10/29 09:33:04, the sun wrote: > nit: for consistency, "SuccessfulItemVerifyFunctor" Done. https://codereview.webrtc.org/1398473004/diff/160001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:118: #if defined(GTEST_HAS_DEATH_TEST) On 2015/10/29 09:33:05, the sun wrote: > Karl use a different condition to enable death tests: > https://codereview.webrtc.org/1408403002/diff/240001/webrtc/base/array_view_u... > > Not sure what is actually required. Can you guys figure out the correct way? Done. https://codereview.webrtc.org/1398473004/diff/160001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:119: TEST(SwapQueueTest, FunctorVerification) { On 2015/10/29 09:33:04, the sun wrote: > UnsuccessfulItemVerifyFunctor Done. https://codereview.webrtc.org/1398473004/diff/160001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:120: IntVerifier verifier(-2); On 2015/10/29 09:33:04, the sun wrote: > IntVerifier is only used in this test - can you move the class in here instead? Done. https://codereview.webrtc.org/1398473004/diff/160001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:131: TEST(SwapQueueTest, UnSuccessfulInvarianceVerification1) { On 2015/10/29 09:33:04, the sun wrote: > UnsuccessfulItemVerifyInsert Done. https://codereview.webrtc.org/1398473004/diff/160001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:141: TEST(SwapQueueTest, UnSuccessfulInvarianceVerification2) { On 2015/10/29 09:33:04, the sun wrote: > UnsuccessfulItemVerifyRemove Done. https://codereview.webrtc.org/1398473004/diff/160001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:182: std::vector<int16_t> template_queue_element(kFrameLength); On 2015/10/29 09:33:04, the sun wrote: > Just make this inline, in the queue instantiation: > SwapQueue<std::vector<int16_t>> queue(kQueueSize, > std::vector<int16_t>(kFrameLength)); Done. https://codereview.webrtc.org/1398473004/diff/160001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:205: TEST(SwapQueueTest, FullQueue) { On 2015/10/29 09:33:05, the sun wrote: > It would be a good test to verify the Remove()d values for a queue that a) runs > full and b) we know will wrap the indices. Either add verification in this test, > or extend MessageTest or VectorTest to wrap around. Done. https://codereview.webrtc.org/1398473004/diff/160001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:238: On 2015/10/29 09:33:04, the sun wrote: > I didn't see a test for Clear(). Added another test case for that. Done.
Great to see this coming together! LGTM with a few more nits. https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... File webrtc/common_audio/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:28: bool NoopSwapQueueVerifierFunction(const T&) { nit: NoopSwapQueueItemVerifierFunction https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:35: // verifcation. drop "item verification" at end of sentence https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:90: // Creates a queue of size size and fills it with the specified number of "Same as above, and accepts a functor..." https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:103: // Creates a queue of size size and fills it with copies of prototype and "Same as above, and accepts a functor..." https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:197: // 0 <= num_elements_ < queue.size() Should that be <= queue.size(). You could remove these comments and put the conditions in code instead. At end of Insert(): RTC_DCHECK_LT(next_write_index, queue.size()); RTC_DCHECK_LE(num_elements_, queue.size()); return true; At end of Remove(): RTC_DCHECK_LT(next_read_index, queue.size()); RTC_DCHECK_LE(num_elements_, queue.size()); https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... File webrtc/common_audio/swap_queue_unittest.cc (right): https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:102: // Ensure that the queue is not overwritten when doing an Insert ?? You're Remove()ing here. https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:167: total nit: could lose a blank line or two here https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:236: std::vector<int16_t> template_queue_element(kFrameLength); unused, remove
https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... File webrtc/common_audio/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:28: bool NoopSwapQueueVerifierFunction(const T&) { On 2015/10/29 12:48:18, the sun wrote: > nit: NoopSwapQueueItemVerifierFunction Done. https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:35: // verifcation. On 2015/10/29 12:48:18, the sun wrote: > drop "item verification" at end of sentence Done. https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:90: // Creates a queue of size size and fills it with the specified number of On 2015/10/29 12:48:18, the sun wrote: > "Same as above, and accepts a functor..." Done. https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:103: // Creates a queue of size size and fills it with copies of prototype and On 2015/10/29 12:48:18, the sun wrote: > "Same as above, and accepts a functor..." Done. https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:197: // 0 <= num_elements_ < queue.size() On 2015/10/29 12:48:18, the sun wrote: > Should that be <= queue.size(). > > You could remove these comments and put the conditions in code instead. > > At end of Insert(): > RTC_DCHECK_LT(next_write_index, queue.size()); > RTC_DCHECK_LE(num_elements_, queue.size()); > return true; > > At end of Remove(): > RTC_DCHECK_LT(next_read_index, queue.size()); > RTC_DCHECK_LE(num_elements_, queue.size()); Done. https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... File webrtc/common_audio/swap_queue_unittest.cc (right): https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:102: // Ensure that the queue is not overwritten when doing an Insert On 2015/10/29 12:48:18, the sun wrote: > ?? You're Remove()ing here. I changed the comment to make it more clear. Done. https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:167: On 2015/10/29 12:48:18, the sun wrote: > total nit: could lose a blank line or two here Done.
https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... File webrtc/common_audio/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:28: bool NoopSwapQueueVerifierFunction(const T&) { On 2015/10/29 12:48:18, the sun wrote: > nit: NoopSwapQueueItemVerifierFunction Done. https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:35: // verifcation. On 2015/10/29 12:48:18, the sun wrote: > drop "item verification" at end of sentence Done. https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:90: // Creates a queue of size size and fills it with the specified number of On 2015/10/29 12:48:18, the sun wrote: > "Same as above, and accepts a functor..." Done. https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:103: // Creates a queue of size size and fills it with copies of prototype and On 2015/10/29 12:48:18, the sun wrote: > "Same as above, and accepts a functor..." Done. https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:197: // 0 <= num_elements_ < queue.size() On 2015/10/29 12:48:18, the sun wrote: > Should that be <= queue.size(). > > You could remove these comments and put the conditions in code instead. > > At end of Insert(): > RTC_DCHECK_LT(next_write_index, queue.size()); > RTC_DCHECK_LE(num_elements_, queue.size()); > return true; > > At end of Remove(): > RTC_DCHECK_LT(next_read_index, queue.size()); > RTC_DCHECK_LE(num_elements_, queue.size()); Done. https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... File webrtc/common_audio/swap_queue_unittest.cc (right): https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:102: // Ensure that the queue is not overwritten when doing an Insert On 2015/10/29 12:48:18, the sun wrote: > ?? You're Remove()ing here. I changed the comment to make it more clear. Done. https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:167: On 2015/10/29 12:48:18, the sun wrote: > total nit: could lose a blank line or two here Done.
https://codereview.webrtc.org/1398473004/diff/200001/webrtc/common_audio/swap... File webrtc/common_audio/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/200001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:180: RTC_DCHECK_GE(next_write_index_, 0U); it's unnecessary to make sure an unsigned type is >= 0...
https://codereview.webrtc.org/1398473004/diff/200001/webrtc/common_audio/swap... File webrtc/common_audio/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/200001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:180: RTC_DCHECK_GE(next_write_index_, 0U); On 2015/10/29 13:40:05, the sun wrote: > it's unnecessary to make sure an unsigned type is >= 0... Done.
https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... File webrtc/common_audio/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:26: // Default item invariance verifier callback function. By virtue of having grown as long as the comment, the function name now makes the comment unnecessary. https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:85: // default constructed Ts. "the specified number of" can be removed. https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:178: bool VerifyQueueContent() { VerifyQueueContents Although it might be better to call it VerifyQueueSlots, so as to not imply that only "full" slots are checked. https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:186: QueueItemVerifier queue_item_verifier_; The verifier is allowed to have mutable state, so it should be GUARDED_BY too. https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:197: // 0 <= num_elements_ < queue.size() On 2015/10/29 12:48:18, the sun wrote: > Should that be <= queue.size(). Yes. (Remember the discussion about whether num_elements could be calculated from the read and write indices? It's not possible precisely because num_elements has one more possible value than the indices.) > You could remove these comments and put the conditions in code instead. > > At end of Insert(): > RTC_DCHECK_LT(next_write_index, queue.size()); > RTC_DCHECK_LE(num_elements_, queue.size()); > return true; > > At end of Remove(): > RTC_DCHECK_LT(next_read_index, queue.size()); > RTC_DCHECK_LE(num_elements_, queue.size()); Sounds like an improvement. Although I usually find it pedagogical to put all the invariants in one place---in comments, as here, or in a method. https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... File webrtc/common_audio/swap_queue_unittest.cc (right): https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:29: TestMessage(const TestMessage& m) : id_(m.id_), info_(m.info_) {} This constructor uses a member initializer list. The other two constructors should, too. Oh, and this is precisely like the copy constructor the compiler would generate for you, so just =default it. https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:38: } Style: I prefer using the non-member form of operators like ==, since that emphasizes that it's symmetrical in the two arguments. (It's perfectly fine to do it this way too, though.) friend bool operator==(const TestMessage& a, const TestMessage& b) { return a.id_ == b.id_ && a.info_ == b.info_; } https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:43: } This swap function won't be found by the standard swap idiom; if you insert debug printouts, you should find that callers use three copies instead (since that's what std::swap falls back to). Do this instead: friend void swap(TestMessage& a, TestMessage& b) { std::swap(id_, m.id_); std::swap(info_, m.info_); } See http://en.cppreference.com/w/cpp/concept/Swappable for details. Hmm. Sine this is test code, and the penalty for copying this class is tiny anyway, you could just skip implementing swap altogether... https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:48: }; Sooo... what are id id and info for? Couldn't you eliminate this whole class, and just use e.g. int instead? https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:100: EXPECT_EQ(i, 2); You should have initialized the empty slots to some easily identifiable value that isn't 2. I know there aren't any empty slots at this point, but that's what you're testing for. https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:102: // Ensure that the queue is not overwritten when doing an Insert On 2015/10/29 12:48:18, the sun wrote: > ?? You're Remove()ing here. He's testing that the Insert on line 99 didn't overwrite. Maybe word it as Ensure that the Insert didn't overwrite anything in the queue. https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:105: EXPECT_EQ(i, 0); To make the test watertight, you should probably examine every element in the queue, not just the frontmost one. https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:126: EXPECT_FALSE(queue.Insert(&i)); Yeah, it doesn't have a size() method, does it? I hadn't thought of that. https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:132: EXPECT_TRUE(queue.Insert(&i)); Or even better, ensure that it's empty by failing to remove. https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:175: SwapQueue<int, IntVerifier> queue(2, verifier); You should be able to do it like this (untested): auto minus_2_verifier = [](const int& i) { return i > -2; } SwapQueue<int, decltype(minus_2_verifier)> queue(2, minus_2_verifier); That'd save ten or so lines. https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:215: messages.push_back(TestMessage(k % 7, k % 17)); LCM(7,17) = 119 < kQueueSize, so you'll have duplicates. Again, why not just use e.g. int? https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:247: kFrameLength * sizeof(samples[0])); buffer_writer.clear(); buffer_writer.insert(buffer_writer.end(), samples + k * kFrameLength, samples + (k + 1) * kFrameLength); or std::copy(samples + k * kFrameLength, samples + (k + 1) * kFrameLength, buffer_writer.begin()); but the second alternative doesn't protect against the input sequence being of a different length than the destination vector. https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:259: I'd like to see tests for 0- and 1-slot queues. They'll provide interesting corner cases.
https://codereview.webrtc.org/1398473004/diff/220001/webrtc/common_audio/swap... File webrtc/common_audio/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/220001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:146: RTC_DCHECK_LE(num_elements_, queue_.size()); You don't check that write index - read index is num elements mod size. https://codereview.webrtc.org/1398473004/diff/220001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:178: RTC_DCHECK_LT(next_read_index_, queue_.size()); You don't check the full set of invariants. Private member function to do this?
https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... File webrtc/common_audio/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:26: // Default item invariance verifier callback function. On 2015/10/29 14:40:35, kwiberg-webrtc wrote: > By virtue of having grown as long as the comment, the function name now makes > the comment unnecessary. :-) Done. https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:85: // default constructed Ts. On 2015/10/29 14:40:35, kwiberg-webrtc wrote: > "the specified number of" can be removed. Done. https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:178: bool VerifyQueueContent() { On 2015/10/29 14:40:35, kwiberg-webrtc wrote: > VerifyQueueContents > > Although it might be better to call it VerifyQueueSlots, so as to not imply that > only "full" slots are checked. Done. https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:186: QueueItemVerifier queue_item_verifier_; On 2015/10/29 14:40:35, kwiberg-webrtc wrote: > The verifier is allowed to have mutable state, so it should be GUARDED_BY too. Done. https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:197: // 0 <= num_elements_ < queue.size() On 2015/10/29 14:40:35, kwiberg-webrtc wrote: > On 2015/10/29 12:48:18, the sun wrote: > > Should that be <= queue.size(). > > Yes. (Remember the discussion about whether num_elements could be calculated > from the read and write indices? It's not possible precisely because > num_elements has one more possible value than the indices.) > > > You could remove these comments and put the conditions in code instead. > > > > At end of Insert(): > > RTC_DCHECK_LT(next_write_index, queue.size()); > > RTC_DCHECK_LE(num_elements_, queue.size()); > > return true; > > > > At end of Remove(): > > RTC_DCHECK_LT(next_read_index, queue.size()); > > RTC_DCHECK_LE(num_elements_, queue.size()); > > Sounds like an improvement. Although I usually find it pedagogical to put all > the invariants in one place---in comments, as here, or in a method. I have rewritten it, please check to see whether it looks ok! https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... File webrtc/common_audio/swap_queue_unittest.cc (right): https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:29: TestMessage(const TestMessage& m) : id_(m.id_), info_(m.info_) {} On 2015/10/29 14:40:35, kwiberg-webrtc wrote: > This constructor uses a member initializer list. The other two constructors > should, too. > > Oh, and this is precisely like the copy constructor the compiler would generate > for you, so just =default it. Done. https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:38: } On 2015/10/29 14:40:35, kwiberg-webrtc wrote: > Style: I prefer using the non-member form of operators like ==, since that > emphasizes that it's symmetrical in the two arguments. (It's perfectly fine to > do it this way too, though.) > > friend bool operator==(const TestMessage& a, const TestMessage& b) { > return a.id_ == b.id_ && a.info_ == b.info_; > } Done. https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:43: } On 2015/10/29 14:40:36, kwiberg-webrtc wrote: > This swap function won't be found by the standard swap idiom; if you insert > debug printouts, you should find that callers use three copies instead (since > that's what std::swap falls back to). Do this instead: > > friend void swap(TestMessage& a, TestMessage& b) { > std::swap(id_, m.id_); > std::swap(info_, m.info_); > } > > See http://en.cppreference.com/w/cpp/concept/Swappable for details. > > Hmm. Sine this is test code, and the penalty for copying this class is tiny > anyway, you could just skip implementing swap altogether... Done. https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:48: }; On 2015/10/29 14:40:35, kwiberg-webrtc wrote: > Sooo... what are id id and info for? Couldn't you eliminate this whole class, > and just use e.g. int instead? Good point! It feels rather redundant. Done. https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:100: EXPECT_EQ(i, 2); On 2015/10/29 14:40:35, kwiberg-webrtc wrote: > You should have initialized the empty slots to some easily identifiable value > that isn't 2. I know there aren't any empty slots at this point, but that's what > you're testing for. That I don't really follow. There is no way to initialize the empty slots to anything other than via inserting items into the queue, right? Otherwise, the slots will be default initialized to zero. In the test I -Initialize the slots to 0 and 1 by inserting those two elements into the 2 element long queue. -Then I try to insert 2, which I check fails according to the spec. -The I check that the value in i has not been swapped to any element from the queue (which I know is either 0 and 1). I cannot see what more I can check with this? https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:102: // Ensure that the queue is not overwritten when doing an Insert On 2015/10/29 14:40:35, kwiberg-webrtc wrote: > On 2015/10/29 12:48:18, the sun wrote: > > ?? You're Remove()ing here. > > He's testing that the Insert on line 99 didn't overwrite. Maybe word it as > > Ensure that the Insert didn't overwrite anything in the queue. Done. https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:105: EXPECT_EQ(i, 0); On 2015/10/29 14:40:35, kwiberg-webrtc wrote: > To make the test watertight, you should probably examine every element in the > queue, not just the frontmost one. Good point! Done. https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:126: EXPECT_FALSE(queue.Insert(&i)); On 2015/10/29 14:40:35, kwiberg-webrtc wrote: > Yeah, it doesn't have a size() method, does it? I hadn't thought of that. Yep, no size check is present. https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:132: EXPECT_TRUE(queue.Insert(&i)); On 2015/10/29 14:40:35, kwiberg-webrtc wrote: > Or even better, ensure that it's empty by failing to remove. I added that as well, and kept the former. It is a bit redundant though. Does that look ok? https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:175: SwapQueue<int, IntVerifier> queue(2, verifier); On 2015/10/29 14:40:35, kwiberg-webrtc wrote: > You should be able to do it like this (untested): > > auto minus_2_verifier = [](const int& i) { return i > -2; } > SwapQueue<int, decltype(minus_2_verifier)> queue(2, minus_2_verifier); > > That'd save ten or so lines. Nice! Done. https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:215: messages.push_back(TestMessage(k % 7, k % 17)); On 2015/10/29 14:40:36, kwiberg-webrtc wrote: > LCM(7,17) = 119 < kQueueSize, so you'll have duplicates. > > Again, why not just use e.g. int? Removed this. Done. https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:236: std::vector<int16_t> template_queue_element(kFrameLength); On 2015/10/29 12:48:18, the sun wrote: > unused, remove Done. https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:247: kFrameLength * sizeof(samples[0])); On 2015/10/29 14:40:36, kwiberg-webrtc wrote: > buffer_writer.clear(); > buffer_writer.insert(buffer_writer.end(), samples + k * kFrameLength, > samples + (k + 1) * kFrameLength); > > or > > std::copy(samples + k * kFrameLength, samples + (k + 1) * kFrameLength, > buffer_writer.begin()); > > but the second alternative doesn't protect against the input sequence being of a > different length than the destination vector. Done. https://codereview.webrtc.org/1398473004/diff/180001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue_unittest.cc:259: On 2015/10/29 14:40:36, kwiberg-webrtc wrote: > I'd like to see tests for 0- and 1-slot queues. They'll provide interesting > corner cases. Done.
https://codereview.webrtc.org/1398473004/diff/240001/webrtc/common_audio/swap... File webrtc/common_audio/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/240001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:179: Why test both read and write, but not num_elements? It's different from Insert...?
https://codereview.webrtc.org/1398473004/diff/240001/webrtc/common_audio/swap... File webrtc/common_audio/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/240001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:179: On 2015/10/29 15:31:09, the sun wrote: > Why test both read and write, but not num_elements? It's different from > Insert...? Done.
Due to the need for reinitializing the queue kwiberg proposed that the queue should support assignment. To that end, I added an overloaded copy assignment operator. Regarding the possibility of using a move assignment operator it seems to be one of the C++11 features that are not ok to use. Could you please again have a look at the new code?
Updated the copy assignment to be thread safe.
https://codereview.webrtc.org/1398473004/diff/300001/webrtc/common_audio/swap... File webrtc/common_audio/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/300001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:109: SwapQueue& operator=(const SwapQueue& o) { 1. This isn't a copy constructor, it is an assignment operator. 2. Using it in the situation it is meant for, will require both construction and assignment of the queue vector. Your current implementation, with a scoped_ptr, only requires construction. So this is a pessimisation. 3. It adds the requirement for items in the queue to be copyable, in addition to being swappable. Please roll this back and land the queue as it is (was). We can investigate adding a *move* assignment operator in the future. https://codereview.webrtc.org/1398473004/diff/300001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:220: mutable rtc::CriticalSection crit_queue_; Remove mutable again? https://codereview.webrtc.org/1398473004/diff/300001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:231: std::vector<T> queue_ GUARDED_BY(crit_queue_); Add DISALLOW_COPY_AND_ASSIGN(SwapQueue);
lgtm with the ill-advised assignment stuff excised https://codereview.webrtc.org/1398473004/diff/300001/webrtc/common_audio/swap... File webrtc/common_audio/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/300001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:109: SwapQueue& operator=(const SwapQueue& o) { On 2015/10/30 09:19:47, the sun wrote: > 1. This isn't a copy constructor, it is an assignment operator. > > 2. Using it in the situation it is meant for, will require both construction and > assignment of the queue vector. Your current implementation, with a scoped_ptr, > only requires construction. So this is a pessimisation. > > 3. It adds the requirement for items in the queue to be copyable, in addition to > being swappable. > > Please roll this back and land the queue as it is (was). We can investigate > adding a *move* assignment operator in the future. Yeah, good point about copying the elements. And move assignment isn't possible until we have the C++11 standard library where std::vector has move support. My fault for not thinking that through. So keep the construction with scoped_ptr---it's probably the least bad option.
Reverted the copy assingment operator. If I still have LGTM, I'll commit. https://codereview.webrtc.org/1398473004/diff/300001/webrtc/common_audio/swap... File webrtc/common_audio/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/300001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:109: SwapQueue& operator=(const SwapQueue& o) { On 2015/10/30 09:19:47, the sun wrote: > 1. This isn't a copy constructor, it is an assignment operator. > > 2. Using it in the situation it is meant for, will require both construction and > assignment of the queue vector. Your current implementation, with a scoped_ptr, > only requires construction. So this is a pessimisation. > > 3. It adds the requirement for items in the queue to be copyable, in addition to > being swappable. > > Please roll this back and land the queue as it is (was). We can investigate > adding a *move* assignment operator in the future. True on all bullets. Did not think of the fact that it imposes a lot of functionality requirements on the queue elements. Rolled back I have. https://codereview.webrtc.org/1398473004/diff/300001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:109: SwapQueue& operator=(const SwapQueue& o) { On 2015/10/30 09:27:29, kwiberg-webrtc wrote: > On 2015/10/30 09:19:47, the sun wrote: > > 1. This isn't a copy constructor, it is an assignment operator. > > > > 2. Using it in the situation it is meant for, will require both construction > and > > assignment of the queue vector. Your current implementation, with a > scoped_ptr, > > only requires construction. So this is a pessimisation. > > > > 3. It adds the requirement for items in the queue to be copyable, in addition > to > > being swappable. > > > > Please roll this back and land the queue as it is (was). We can investigate > > adding a *move* assignment operator in the future. > > Yeah, good point about copying the elements. And move assignment isn't possible > until we have the C++11 standard library where std::vector has move support. My > fault for not thinking that through. > > So keep the construction with scoped_ptr---it's probably the least bad option. Acknowledged. https://codereview.webrtc.org/1398473004/diff/300001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:220: mutable rtc::CriticalSection crit_queue_; On 2015/10/30 09:19:47, the sun wrote: > Remove mutable again? Done. https://codereview.webrtc.org/1398473004/diff/300001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:231: std::vector<T> queue_ GUARDED_BY(crit_queue_); On 2015/10/30 09:19:47, the sun wrote: > Add > DISALLOW_COPY_AND_ASSIGN(SwapQueue); Done.
Description was changed from ========== Changed queue implementation to the proposed vector-based solution. Added unit tests. BUG=webrtc:5099 ========== to ========== Changed queue implementation to the proposed vector-based solution. Added unit tests. BUG=webrtc:5099 TBR=hlundin-webrtc ==========
For the record (although I've said it before): lgtm
Still LGTM. https://codereview.webrtc.org/1398473004/diff/420001/webrtc/common_audio/swap... File webrtc/common_audio/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/420001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:88: // item verification functor. nit: "Same as above and accepts an item verification functor." https://codereview.webrtc.org/1398473004/diff/420001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:99: // Same as above and accepts a functor to use for initializing the item nit: "Same as above and accepts an item verification functor." Also nit: since we say "same as above", it feels a bit weird that the item verifier argument isn't last. Fix if you wish. https://codereview.webrtc.org/1398473004/diff/420001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:194: QueueItemVerifier queue_item_verifier_ GUARDED_BY(crit_queue_); Add a: // TODO(peah): Change this to use std::function() once we can use C++11 std lib.
https://codereview.webrtc.org/1398473004/diff/420001/webrtc/common_audio/swap... File webrtc/common_audio/swap_queue.h (right): https://codereview.webrtc.org/1398473004/diff/420001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:88: // item verification functor. On 2015/11/09 11:55:19, the sun wrote: > nit: "Same as above and accepts an item verification functor." Done. https://codereview.webrtc.org/1398473004/diff/420001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:99: // Same as above and accepts a functor to use for initializing the item On 2015/11/09 11:55:19, the sun wrote: > nit: "Same as above and accepts an item verification functor." > > Also nit: since we say "same as above", it feels a bit weird that the item > verifier argument isn't last. Fix if you wish. Done. https://codereview.webrtc.org/1398473004/diff/420001/webrtc/common_audio/swap... webrtc/common_audio/swap_queue.h:194: QueueItemVerifier queue_item_verifier_ GUARDED_BY(crit_queue_); On 2015/11/09 11:55:18, the sun wrote: > Add a: > // TODO(peah): Change this to use std::function() once we can use C++11 std lib. Done.
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org, solenberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/1398473004/#ps440001 (title: "Comment changes and modified the order of parameters for one of the constructors")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1398473004/440001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1398473004/440001
Message was sent while issue was closed.
Patchset 23 (id:??) landed as https://crrev.com/48407f71221635aec2d2a382ee24dea28e4f69cf Cr-Commit-Position: refs/heads/master@{#10562}
Message was sent while issue was closed.
Committed patchset #23 (id:440001) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
