Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
XibrenX opened this issue Jan 10, 2024 · 5 comments
Assignees

Comments

@XibrenX
Copy link

XibrenX commented Jan 10, 2024

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
Copy link
Contributor

It seems the algorithm works fine without the initial find_if.

@enbyted
Copy link

enbyted commented Apr 22, 2024

@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
Copy link
Contributor

Thanks, I'll take a look at that.

@jwellbelove
Copy link
Contributor

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 pushed a commit that referenced this issue Apr 24, 2024
@jwellbelove
Copy link
Contributor

Fixed 20.38.12

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests

3 participants