etl icon indicating copy to clipboard operation
etl copied to clipboard

etl::remove_if calls predicate twice for first iterator, where predicate returns true.

Open XibrenX opened this issue 1 year ago • 1 comments

See code below. etl::find_if returns the first iterator, where predicate returns true. So if first != last, predicate is already called for *first. However, itr = first and if (!predicate(*itr)) calls predicate again for *first.

In our code, we have logging for when an item in the iterator has been marked for removal. Resulting in logging this twice for the first element that is marked for removal.

  /// Remove If
  ///\ingroup algorithm
  //***************************************************************************
  template <typename TIterator, typename TUnaryPredicate>
  ETL_CONSTEXPR14
  TIterator remove_if(TIterator first, TIterator last, TUnaryPredicate predicate)
  {
    first = etl::find_if(first, last, predicate);

    if (first != last)
    {
      TIterator itr = first;

      while (itr != last)
      {
        if (!predicate(*itr))
        {
          *first = etl::move(*itr);
          ++first;
        }

        ++itr;
      }
    }

    return first;
  }

XibrenX avatar Jan 10 '24 12:01 XibrenX

It seems the algorithm works fine without the initial find_if.

jwellbelove avatar Jan 11 '24 14:01 jwellbelove

@jwellbelove Yes, the algorithm works without the find if, however in case the predicate has a small chance of returning true (i.e. you have a lot of items and want to erase only a small amount) it causes a lot of unnecessary moves, especially if you are trying to remove only items close to the end of collection.

Please consider using something similar to the possible implementation on cppreference: https://en.cppreference.com/w/cpp/algorithm/remove Notice the clever trick with ++i in the for loop comparison.

enbyted avatar Apr 22 '24 10:04 enbyted

Thanks, I'll take a look at that.

jwellbelove avatar Apr 22 '24 12:04 jwellbelove

I went back to the original implementation and just moved the increment.

    first = etl::find_if(first, last, predicate);

    if (first != last)
    {
      TIterator itr = first;

      while (++itr != last)
      {
        if (!predicate(*itr))
        {
          *first++ = etl::move(*itr);
        }
      }
    }

    return first;

jwellbelove avatar Apr 23 '24 08:04 jwellbelove

Fixed 20.38.12

jwellbelove avatar May 31 '24 20:05 jwellbelove