Using STL algorithms with shared_ptr, function objects

Question

I have a set of shared_ptr, and I'd like to use remove_copy_if with a custom function object for the predicate. I didn't know the "best" way to do it. Right now, I've got this working:

class CellInCol : public std::unary_function<const std::shared_ptr<Cell>,
                                             bool>
{
public:
    CellInCol( size_t col ) : _col( col ) {}
    bool operator() ( const std::shared_ptr<Cell> &a ) const
    {
        return ( a->GetX() == _col );
    }
private:
    size_t _col;
};


    typedef std::set<std::shared_ptr<Cell>, CellSorter> Container;
    Container _grid;
    // initialization omitted...


Puzzle::Container Puzzle::GetCol( size_t c )
{
    Cell::Validate( c, 1, 9 );
    Container col;
    std::remove_copy_if( _grid.begin(), _grid.end(),
                         std::inserter( col, col.begin() ),
                         std::not1( CellInCol( c ) ) );
    return col;
}

I decided to do const references to shared_ptr because the object won't hold on to the pointer and this just seemed more efficient than an extra copy of the shared_ptr.

It seems like it would be better to just take const references to the objects, but I couldn't get it to compile. I changed it to this, but no luck:

class CellInCol : public std::unary_function<const Cell,
                                             bool>
{
public:
    CellInCol( size_t col ) : _col( col ) {}

    // note use of const ref to shared_ptr's
    bool operator() ( const Cell &a ) const
    {
        return ( a.GetX() == _col );
    }
private:
    size_t _col;
};

Here is the output from g++:

In file included from /usr/include/c++/4.4/algorithm:62,
                 from /usr/include/c++/4.4/valarray:41,
                 from Puzzle.h:5,
                 from Puzzle.cpp:2:
/usr/include/c++/4.4/bits/stl_algo.h: In function ‘_OIter std::remove_copy_if(_IIter, _IIter, _OIter, _Predicate) [with _IIter = std::_Rb_tree_const_iterator<std::shared_ptr<Sudoku::Cell> >, _OIter = std::insert_iterator<std::set<std::shared_ptr<Sudoku::Cell>, Sudoku::CellSorter, std::allocator<std::shared_ptr<Sudoku::Cell> > > >, _Predicate = std::unary_negate<Sudoku::<unnamed>::CellInRow>]’:
Puzzle.cpp:100:   instantiated from here
/usr/include/c++/4.4/bits/stl_algo.h:938: error: no match for call to ‘(std::unary_negate<Sudoku::<unnamed>::CellInRow>) (const std::shared_ptr<Sudoku::Cell>&)’
/usr/include/c++/4.4/bits/stl_function.h:357: note: candidates are: bool std::unary_negate<_Predicate>::operator()(const typename _Predicate::argument_type&) const [with _Predicate = Sudoku::<unnamed>::CellInRow]
/usr/include/c++/4.4/bits/stl_algo.h: In function ‘_OIter std::remove_copy_if(_IIter, _IIter, _OIter, _Predicate) [with _IIter = std::_Rb_tree_const_iterator<std::shared_ptr<Sudoku::Cell> >, _OIter = std::insert_iterator<std::set<std::shared_ptr<Sudoku::Cell>, Sudoku::CellSorter, std::allocator<std::shared_ptr<Sudoku::Cell> > > >, _Predicate = std::unary_negate<Sudoku::<unnamed>::CellInCol>]’:
Puzzle.cpp:110:   instantiated from here
/usr/include/c++/4.4/bits/stl_algo.h:938: error: no match for call to ‘(std::unary_negate<Sudoku::<unnamed>::CellInCol>) (const std::shared_ptr<Sudoku::Cell>&)’
/usr/include/c++/4.4/bits/stl_function.h:357: note: candidates are: bool std::unary_negate<_Predicate>::operator()(const typename _Predicate::argument_type&) const [with _Predicate = Sudoku::<unnamed>::CellInCol]
/usr/include/c++/4.4/bits/stl_algo.h: In function ‘_OIter std::remove_copy_if(_IIter, _IIter, _OIter, _Predicate) [with _IIter = std::_Rb_tree_const_iterator<std::shared_ptr<Sudoku::Cell> >, _OIter = std::insert_iterator<std::set<std::shared_ptr<Sudoku::Cell>, Sudoku::CellSorter, std::allocator<std::shared_ptr<Sudoku::Cell> > > >, _Predicate = std::unary_negate<Sudoku::<unnamed>::CellInBlock>]’:
Puzzle.cpp:121:   instantiated from here
/usr/include/c++/4.4/bits/stl_algo.h:938: error: no match for call to ‘(std::unary_negate<Sudoku::<unnamed>::CellInBlock>) (const std::shared_ptr<Sudoku::Cell>&)’
/usr/include/c++/4.4/bits/stl_function.h:357: note: candidates are: bool std::unary_negate<_Predicate>::operator()(const typename _Predicate::argument_type&) const [with _Predicate = Sudoku::<unnamed>::CellInBlock]
/usr/include/c++/4.4/bits/stl_algo.h: In function ‘_OIter std::remove_copy_if(_IIter, _IIter, _OIter, _Predicate) [with _IIter = std::_Rb_tree_const_iterator<std::shared_ptr<Sudoku::Cell> >, _OIter = std::insert_iterator<std::set<std::shared_ptr<Sudoku::Cell>, Sudoku::CellSorter, std::allocator<std::shared_ptr<Sudoku::Cell> > > >, _Predicate = std::unary_negate<Sudoku::<unnamed>::CellIsNeighbor>]’:
Puzzle.cpp:154:   instantiated from here
/usr/include/c++/4.4/bits/stl_algo.h:938: error: no match for call to ‘(std::unary_negate<Sudoku::<unnamed>::CellIsNeighbor>) (const std::shared_ptr<Sudoku::Cell>&)’
/usr/include/c++/4.4/bits/stl_function.h:357: note: candidates are: bool std::unary_negate<_Predicate>::operator()(const typename _Predicate::argument_type&) const [with _Predicate = Sudoku::<unnamed>::CellIsNeighbor]
make: *** [Puzzle.o] Error 1

Is there another way to do it, or any suggestions?

Answer 1

First of all, since you're using the C++0x features (std::shared_ptr), it makes sense to use std::copy_if() to avoid having to call std::not1.

The first functor you wrote works, and a minimal compilable example would be something like this: https://ideone.com/XhuNu

The second functor does not work, as the compiler points out, due to mismatch between its argument_type (which is const Cell) and the argument that it is being called with, which is const std::shared_ptr<Cell>&.

It's simply not what the container contains! For all it knows at this point, those Cell objects may not even be copyable.

The second functor would indeed be a better thing to use if the container is a set of Cells, not a set of shared pointers to Cells. It is considered good design to avoid shared ownership of objects anyway.

Example code that would compile with the second functor

#include <set>
#include <functional>
#include <algorithm>
#include <iostream>

struct Cell {
        int mX;
        Cell(int x) : mX(x) {}
        size_t GetX() const { return mX;}
};
struct CellSorter {
        bool operator()(const Cell& l, const Cell& r) const
        {
                return l.GetX() < r.GetX();
        }
};

// your second functor begins
class CellInCol : public std::unary_function<const Cell,
                                             bool>
{
public:
    CellInCol( size_t col ) : _col( col ) {}

    // note use of const ref to shared_ptr's
    bool operator() ( const Cell &a ) const
    {
        return ( a.GetX() == _col );
    }
private:
    size_t _col;
};
// your second functor ends

int main()
{
    typedef std::set<Cell, CellSorter> Container;
    Container _grid = {Cell(1), Cell(2), Cell(7), Cell(10)};
    Container col;
    size_t c = 7;
    std::remove_copy_if( _grid.begin(), _grid.end(),
                         std::inserter( col, col.begin() ),
                         std::not1( CellInCol( c ) ) );
    std::cout << "col has " << col.size() << " elements\n"
              << "the first element is " << col.begin()->GetX() << '\n';
}

test run: https://ideone.com/kLiFn

Answer 2

You could use boost::make_indirect_iterator to make std::remove_copy_if work on the Cells instead of the shared_ptrs. However, since the algorithm would be working on the Cells directly, the output iterator would also have to take Cells and not shared_ptrs. Which means the output collection would have to be a collection of Cells.

If you want to store shared_ptrs, you'd have to transform the predicate somehow. You can use boost::lambda to do just that.

Cubbi's example modified to use boost::lambda:

#include <memory>
#include <set>
#include <functional>
#include <algorithm>
#include <iostream>
#include <boost/shared_ptr.hpp>
#include <boost/lambda/lambda.hpp>
#include <boost/lambda/bind.hpp>


struct Cell {
        int mX;
        Cell(int x) : mX(x) {}
        size_t GetX() const { return mX;}
};
struct CellSorter {
        bool operator()(const boost::shared_ptr<Cell>& l, const boost::shared_ptr<Cell>& r) const
        {
                return l->GetX() < r->GetX();
        }
};

class CellInCol : public std::unary_function<Cell, bool>
{
public:
    CellInCol( size_t col ) : _col( col ) {}

    // note use of const ref to shared_ptr's
    bool operator() ( const Cell &a ) const
    {
        return ( a.GetX() == _col );
    }
private:
    size_t _col;
};

int main()
{
    typedef std::set<boost::shared_ptr<Cell>, CellSorter> Container;
    Container _grid;
    _grid.insert( boost::shared_ptr<Cell>(new Cell(1)));
    _grid.insert( boost::shared_ptr<Cell>(new Cell(2)));
    _grid.insert( boost::shared_ptr<Cell>(new Cell(7)));
    _grid.insert( boost::shared_ptr<Cell>(new Cell(10)));
    Container col;
    size_t c = 7;

    std::remove_copy_if(
        _grid.begin(),
        _grid.end(),
        std::inserter( col, col.begin() ),
        !boost::lambda::bind(CellInCol(c), *boost::lambda::_1) // <------ :^)
    );

    std::cout << "col has " << col.size() << " elements\n"
              << " the first element is " << (*col.begin())->GetX() << '\n';
}

(ideone's C++0x compiler doesn't know it's boost, so I changed std::shared_ptr to boost::shared_ptr, but that should make no difference)

http://www.ideone.com/mtMUj

ps:

I decided to do const references to shared_ptr because the object won't hold on to the pointer and this just seemed more efficient than an extra copy of the shared_ptr.

Yes, you should (almost) always pass shared_ptrs as reference-to-const, it makes a huge difference. Copying a shared_ptr on a platform with threads means at least one atomic instruction (CAS, atomic-increment or something similar), and those can be rather expensive. (And of course destroying the copy will be equally expensive)

The only exception I can think of would be if the function will copy the shared_ptr. In that case you could either take it by value and use swap() to "copy" it, or provide an rvalue-reference overload. (If the function doesn't always copy the shared_ptr, the rvalue-reference overload would be the preferred solution).

Of course it doesn't make a big difference if the function is expensive anyway, but if it's a very cheap function that might get inlined and is called in a thight loop, the difference can be quite noticeable.