ETLCPP / etl

Embedded Template Library
https://www.etlcpp.com
MIT License
2.17k stars 386 forks source link

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

Closed XibrenX closed 4 months ago

XibrenX commented 8 months ago

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;
  }
jwellbelove commented 8 months ago

It seems the algorithm works fine without the initial find_if.

enbyted commented 5 months ago

@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.

jwellbelove commented 5 months ago

Thanks, I'll take a look at that.

jwellbelove commented 5 months ago

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 commented 4 months ago

Fixed 20.38.12