6

I need to pass a derived comparator to std::priority_queue, but for some reason the base class' operator() is being called.

Here is a minimal code that shows this behavior:

class Base {
    public:
    virtual bool operator() (int l, int r) const {
        cout << "Should not be called" << std::endl;
        return 0;
    }
    virtual ~Base() {}
};
class A : public Base { 
    public:
    bool operator() (int l, int r) const override {
        cout << "Should be called!!!!";
        return l < r;
    }
};
int main() {
    priority_queue<int, vector<int>, Base> pq((A()));
    pq.push(1);
    pq.push(2);
    pq.push(3);
    pq.push(0);
    cout << pq.top();
    return 0;
}

The code is available on ideone as well

Note that I cannot use priority_queue<int, vector<int>, A>, because I have other subclasses for Base, and that will result in a lot of code duplication1.

What am I doing wrong? How can I pass a comparator to the priority_queue that will be used during its life time?


(1) I know I can bypass the code duplication issue by using template functions that accept priority_queue<int,vector<int>, T> - but I really rather not to.

amit
  • 175,853
  • 27
  • 231
  • 333
  • @πάνταῥεῖ Why slicing? The constructor is accepting `const Compare&`. I am reluctant to believe it is later saved by value in a std library class. – amit Feb 25 '16 at 18:25
  • Sorry en.cppreference.com was down for a moment. – πάντα ῥεῖ Feb 25 '16 at 18:27
  • 4
    @amit It has to be saved by value internally. If the container was just saving a reference, the code you have in the question would be undefined behavior because the lifetime of the comparator instance you pass to the container ends when the constructor call returns. – Praetorian Feb 25 '16 at 18:37
  • 1
    Of course it's saved by value. – Jonathan Wakely Feb 25 '16 at 19:26

2 Answers2

8

The standard specifies Compare comp as value members of the class template in 23.6.4.1. The constructors are said to:

Initializes comp with x and c with y (copy constructing or move constructing as appropriate);

Therefore you have slicing, even if the parameter type is actually a const Compare&.

To work around that, you could implement a pimpl-wrapper for the comparator. This wrapper would internally keep a Base& to the actual comparator, and in it's non-virtual operator() simply call the virtual operator() of the Base / A comparator.

Please think carefully about the lifetime of your A Object. Depending on the needed state of your Comparator, you could implement a virtual clone-method in Base. And keep Base as a std::unique_ptr<Base> in your PimplCompare - which you clone in it's copy-ctor. Or you keep it as std::shared_ptr<Base>.

Community
  • 1
  • 1
Zulan
  • 21,896
  • 6
  • 49
  • 109
  • I believe [cppreference.com](http://en.cppreference.com/w/cpp/container/priority_queue/priority_queue) is in error when it states for (2) "Move-constructs the comparison functor comp with std::move(compare). In all constructors, compare is handed by `const Comp&`. `std::move(compare)` would be of type `const Comp&&`, but the move-constructor would take a `Comp&&` - so it would call the copy-ctor. libstdc++ does not seem to do any moving on the comparators. – Zulan Feb 25 '16 at 19:02
5

The constructor takes a const Compare& which would not cause any slicing when passing the object to the function but we then have in the documentation

Copy-constructs the underlying container c with the contents of cont. Copy-constructs the comparison functor comp with the contents of compare.

Since a copy is happening and the template type is Base you are only going to copy and store the Base part of the A object.

You will have to wrap the comparison object in some sort of wrapper and expose a non virtual operator () that will call the virtual operator() of the type passed to the priority_queue constructor.

NathanOliver
  • 171,901
  • 28
  • 288
  • 402
  • 1
    _"you are only going to copy and store the Base part of the A object."_ Isn't that exactly _slicing_? – πάντα ῥεῖ Feb 25 '16 at 18:29
  • The question is "What am I doing wrong?". Or to be more explicit, how can I pass a comparator to the priority_queue that will be used during its life time? – amit Feb 25 '16 at 18:30
  • @πάνταῥεῖ I reworded it to clear up that I meant it was not slicing when be ing passed to the function. – NathanOliver Feb 25 '16 at 18:31