[feat] Addressed code review

src/feat/online-feature-test.cc

@@ -381,13 +381,15 @@ void TestRecyclingVector() {
  for (int i = 0; i != 100; ++i) {
  Vector <BaseFloat> data(1);
  data.Set(i);
- full_vec.Store(new Vector<BaseFloat>(data));
- shrinking_vec.Store(new Vector<BaseFloat>(data));
+ full_vec.PushBack(new Vector<BaseFloat>(data));
+ shrinking_vec.PushBack(new Vector<BaseFloat>(data));
  }
+ KALDI_ASSERT(full_vec.Size() == 100);
+ KALDI_ASSERT(shrinking_vec.Size() == 100);
 
  // full_vec should contain everything
  for (int i = 0; i != 100; ++i) {
- Vector <BaseFloat> *data = full_vec.Retrieve(i);
+ Vector <BaseFloat> *data = full_vec.At(i);
  KALDI_ASSERT(data != nullptr);
  KALDI_ASSERT((*data)(0) == static_cast<BaseFloat>(i));
  }
@@ -396,7 +398,7 @@ void TestRecyclingVector() {
  int caught_exceptions = 0;
  for (int i = 0; i != 90; ++i) {
  try {
- shrinking_vec.Retrieve(i);
+ shrinking_vec.At(i);
  } catch (const std::runtime_error &) {
  ++caught_exceptions;
  }
@@ -406,7 +408,7 @@ void TestRecyclingVector() {
 
  // shrinking_vec should contain the last 10 elements
  for (int i = 90; i != 100; ++i) {
- Vector <BaseFloat> *data = shrinking_vec.Retrieve(i);
+ Vector <BaseFloat> *data = shrinking_vec.At(i);
  KALDI_ASSERT(data != nullptr);
  KALDI_ASSERT((*data)(0) == static_cast<BaseFloat>(i));
  }

src/feat/online-feature.cc

@@ -24,14 +24,18 @@
 
 namespace kaldi {
 
-RecyclingVector::RecyclingVector(int items_to_hold) : items_to_hold_(items_to_hold),
- first_available_index_(0) { }
+RecyclingVector::RecyclingVector(int items_to_hold) :
+ items_to_hold_(items_to_hold == 0 ? -1 : items_to_hold),
+ first_available_index_(0) {
+}
 
 RecyclingVector::~RecyclingVector() {
- DeletePointers(&items_);
+ for (auto *item : items_) {
+ delete item;
+ }
 }
 
-Vector<BaseFloat> *RecyclingVector::Retrieve(int index) const {
+Vector<BaseFloat> *RecyclingVector::At(int index) const {
  if (index < first_available_index_) {
  KALDI_ERR << "Attempted to retrieve feature vector that was "
  "already removed by the RecyclingVector (index = " << index << "; "
@@ -42,16 +46,11 @@ Vector<BaseFloat> *RecyclingVector::Retrieve(int index) const {
  return items_.at(index - first_available_index_);
 }
 
-void RecyclingVector::Store(Vector<BaseFloat> *item) {
- // Internally, we keep at most 2x requested items_to_hold_ and clean up
- // when this number is reached
- const int size_before_erase = items_.size();
- if (size_before_erase == 2 * items_to_hold_) {
- for (int i = 0; i != items_to_hold_; ++i) {
- delete items_[i];
- }
- items_.erase(items_.begin(), items_.begin() + items_to_hold_);
- first_available_index_ += (size_before_erase - items_.size());
+void RecyclingVector::PushBack(Vector<BaseFloat> *item) {
+ if (items_.size() == items_to_hold_) {
+ delete items_.front();
+ items_.pop_front();
+ ++first_available_index_;
  }
  items_.push_back(item);
 }
@@ -64,7 +63,7 @@ int RecyclingVector::Size() const {
 template<class C>
 void OnlineGenericBaseFeature<C>::GetFrame(int32 frame,
  VectorBase<BaseFloat> *feat) {
- feat->CopyFromVec(*(features_.Retrieve(frame)));
+ feat->CopyFromVec(*(features_.At(frame)));
 };
 
 template<class C>
@@ -117,7 +116,7 @@ void OnlineGenericBaseFeature<C>::ComputeFeatures() {
  // note: this online feature-extraction code does not support VTLN.
  BaseFloat vtln_warp = 1.0;
  computer_.Compute(raw_log_energy, vtln_warp, &window, this_feature);
- features_.Store(this_feature);
+ features_.PushBack(this_feature);
  }
  // OK, we will now discard any portion of the signal that will not be
  // necessary to compute frames in the future.

src/feat/online-feature.h

@@ -52,16 +52,20 @@ class RecyclingVector {
  /// By default it does not remove any elements.
  RecyclingVector(int items_to_hold = -1);
 
- Vector<BaseFloat> *Retrieve(int index) const;
+ /// The ownership is being retained by this collection - do not delete the item.
+ Vector<BaseFloat> *At(int index) const;
 
- void Store(Vector<BaseFloat> *item);
+ /// The ownership of the item is passed to this collection - do not delete the item.
+ void PushBack(Vector<BaseFloat> *item);
 
+ /// This method returns the size as if no "recycling" had happened,
+ /// i.e. equivalent to the number of times the PushBack method has been called.
  int Size() const;
 
  ~RecyclingVector();
 
 private:
- std::vector<Vector<BaseFloat>*> items_;
+ std::deque<Vector<BaseFloat>*> items_;
  int items_to_hold_;
  int first_available_index_;
 };