0

I've followed another question to create a template function that removes a member from a vector by value, however when I try to compile it I'm getting this error:

/usr/include/c++/11/bits/predefined_ops.h: In instantiation of ‘bool __gnu_cxx::__ops::_Iter_equals_val<_Value>::operator()(_Iterator) [with _Iterator = __gnu_cxx::__normal_iterator<Types::segment*, std::vector<Types::segment> >; _Value = const Types::segment]’:
/usr/include/c++/11/bits/stl_algo.h:822:13:   required from ‘_ForwardIterator std::__remove_if(_ForwardIterator, _ForwardIterator, _Predicate) [with _ForwardIterator = __gnu_cxx::__normal_iterator<Types::segment*, std::vector<Types::segment> >; _Predicate = __gnu_cxx::__ops::_Iter_equals_val<const Types::segment>]’
/usr/include/c++/11/bits/stl_algo.h:860:30:   required from ‘_FIter std::remove(_FIter, _FIter, const _Tp&) [with _FIter = __gnu_cxx::__normal_iterator<Types::segment*, std::vector<Types::segment> >; _Tp = Types::segment]’
/home/turgut/Desktop/CppProjects/videoo-render/src/utils/array_man.h:49:34:   required from ‘void Utils::remove_element(std::vector<T>*, seek) [with vector_type = Types::segment; seek = Types::segment]’
/home/turgut/Desktop/CppProjects/videoo-render/src/Application.cpp:184:59:   required from here
/usr/include/c++/11/bits/predefined_ops.h:270:24: error: no match for ‘operator==’ (operand types are ‘Types::segment’ and ‘const Types::segment’)
  270 |         { return *__it == _M_value; }

Here is what my function looks like:

template<typename vector_type, typename seek>
    void remove_element(std::vector<vector_type>* vector, seek obj)
    {
        vector->erase(std::remove(vector->begin(), vector->end(), (seek)obj), vector->end());
    }

And here is how I use it:

for(auto segment: opengl_engine->_textures){
  if(must_delete){
    Utils::remove_element<Types::segment, Types::segment>(&(opengl_engine->_textures), 
    segment);
  }
}
Turgut
  • 711
  • 3
  • 25
  • 4
    `operand types are ‘Types::segment’ and ‘const Types::segment’` You have a type mis-match. – sweenish Aug 23 '22 at 13:52
  • 6
    It looks like `Types::segment` doesn't have the correct `operator==`. Possibly the parameter to that one is not `const` – perivesta Aug 23 '22 at 13:53
  • 1
    Having the two template parameters feels redundant. At least I'm not seeing the need for it, generally. – sweenish Aug 23 '22 at 13:56
  • 2
    @NateEldredge The erase/remove idiom is pretty well known and even recommended on cppreference. `vector->end()` isn't affected by what `std::remove()` does. – sweenish Aug 23 '22 at 13:56
  • 1
    @sweenish: Oh I see, `std::remove` maintains the size but leaves the elements at the end filled with garbage. Which `vector->erase` then cleans up. – Nate Eldredge Aug 23 '22 at 14:00
  • 1
    Unrelated to compilation error, but you modifying vector you iterating over using for range loop, that will not work. And you code looks strange, you iterate over and then check flag. You should just use that flag inside `std::remove_if()` instead – Slava Aug 23 '22 at 14:00
  • 1
    @Slava The loop is hitting a different vector at each iteration. The size of the vector being looped on is not affected. There is no iterator invalidation happening here. – sweenish Aug 23 '22 at 14:08
  • 1
    @sweenish what do you mean different vector? For is looping over `opengl_engine->_textures` and pointer to `opengl_engine->_textures` is passed to `Utils::remove_element()`. Where do you see different vectors? – Slava Aug 23 '22 at 14:11
  • 1
    @sweenish Can you please show me where exactly I must change? I tried changing `Utils::remove_element` to no avail. – Turgut Aug 23 '22 at 14:12
  • 1
    @Slava You're right, I misread. I thought it was a vector of vectors. If OP wants all instances removed, they should just use `std::remove_if()`. – sweenish Aug 23 '22 at 14:13
  • 1
    @Turgut you need to rethink your code, fixing compilation error would not do anything, your approach is broken. Tells what you are trying to achieve, as your code is difficult to understand. For example why do you check `if(must_delete)` inside your loop every time? – Slava Aug 23 '22 at 14:15
  • 1
    @Slava My goal is to remove the used segment from the vector once they have fulfilled their purposes. I made a template because I'm going to use this function on different places. – Turgut Aug 23 '22 at 14:17
  • 1
    @sweenish Can you provide an example code similar to my use case please? – Turgut Aug 23 '22 at 14:17
  • 1
    @Turgut remove/erase idiom created so you can move all objects that suppose to be erased to the end and then erase them all at one shot, that is more efficient. You use that in a loop for one by one element, which completely defeats purpose of this idiom and made your code even worse and more convoluted. Your whole loop should be replaced with `std::remove_if` not to call `std::remove` from inside that loop. Where flag `must_delete` comes from? – Slava Aug 23 '22 at 14:21
  • 1
    @Slava All segments have a timer, and each 60 frames that counter goes up, when the timer reaches the segments max duration, that segment must end thus I must remove it from the vector, so it is not red any further. These segments are videos decoded by ffmpeg. – Turgut Aug 23 '22 at 14:24
  • 1
    Sorry for misunderstanding, I am not asking how it works, I am asking how you literally calculate this flag `must_delete`? Do you call a function? If yes that function is the predicate that you should use in `std::remove_if()`. I cannot replace your loop for you with `std::remove_if()` as using this flag would be pointless in the new code. – Slava Aug 23 '22 at 14:27

1 Answers1

2

Here is a toy example that does what you're trying to do.

#include <iostream>
#include <list>
#include <vector>

template <typename Container>
void remove_all_by_value(Container& c, typename Container::value_type val) {
  c.erase(std::remove_if(c.begin(), c.end(),
                         [&val](const auto& a) { return a == val; }),
          c.end());
}

template <typename Container>
void print_container(const Container& c) {
  for (const auto& i : c) {
    std::cout << i << ' ';
  }
  std::cout << '\n';
}

int main() {
  std::vector<int> one{1, 2, 3, 2, 4, 2, 5};
  print_container(one);
  remove_all_by_value(one, 2);
  print_container(one);

  std::cout << '\n';

  std::list<int> two{1, 2, 3, 2, 4, 2, 5};
  print_container(two);
  remove_all_by_value(two, 2);
  print_container(two);
}

std::remove_if will remove all objects that match the criteria in the provided lambda. This removes the need to have a loop at all.

I haven't been bothered to install a C++20 compiler yet, so it's very likely that there exists a more elegant version that utilizes concepts and ranges.

sweenish
  • 4,793
  • 3
  • 12
  • 23