How do I turn relational comparison of pointers into an error?

Question

We've been bitten by the following bug many times:

#include <iostream>
#include <vector>
#include <algorithm>

using namespace std;

void print(int* pn) { cout << *pn << " "; }

int main() {
    int* n1 = new int(1);
    int* n2 = new int(2);
    int* n3 = new int(3);

    vector<int*> v;
    v.push_back(n1);
    v.push_back(n2);
    v.push_back(n3);

    sort(v.begin(), v.end());   // Here be dragons!

    for_each(v.begin(), v.end(), print);
    cout << endl;
    delete n1; delete n2; delete n3;
}

The problem is that std::sort is comparing integer pointers not integers, which is not what the programmer intended. Worse, the output may appear correct and deterministic (consider the order of addresses returned by new or allocated on the stack). The root problem is that sort eventually calls operator< for T, which is rarely a good idea when T is a pointer type.

Is there any way to prevent this or at least get a compiler warning? For example, is there a way to create a custom version of std::sort that requires a comparison function when T is a pointer?

Answer 1

IMO, the programmers should know that std::sort assumes the container stores values. If you need a different behavior for the comparison, then you provide a comparison function. E.g. (untested):

template<typename T>
inline bool deref_compare(T* t1, T* t2) { return *t1 < *t2; }

//...

std::sort(v.begin(), v.end(), deref_compare<int>);

Edit

FWIW, Jacob's answer comes closest to directly accomplishing what you want. There might be some ways to generalize it further.

Answer 2

I don't have a good answer for pointers in general, but you can restrict comparisons if you're using a smart pointer of any kind - eg boost::shared_ptr.

#include <boost/shared_ptr.hpp>
using namespace std;

template<class T>
bool operator<(boost::shared_ptr<T> a, boost::shared_ptr<T> b)
{
  return boost::shared_ptr<T>::dont_compare_pointers;
}

int main () {
  boost::shared_ptr<int> A;
  boost::shared_ptr<int> B;
  bool i = A < B;  
}

Output:

In function 'bool operator<(boost::shared_ptr<T>, boost::shared_ptr<T>) [with T = int]':
t.cpp:15:   instantiated from here
Line 8: error: 'dont_compare_pointers' is not a member of 'boost::shared_ptr<int>'
compilation terminated due to -Wfatal-errors.

So you can use smart pointers, or create your own smart pointer wrapper. This is very heavyweight for what you want though, so if you do create a wrapper to detect this situation, I recommend you only use it in debug mode. So create a macro (ugh, I know) and use it to declare pointers.

#ifdef DEBUG
    #define pointer(x) pointer_wrapper<X>
#else
    #define pointer(x) x*
#endif

This still requires your programmers to use it, of course!

Answer 3

For pointers in general you could do this:

    #include <ctime>
    #include <vector>
    #include <cstdlib>
    #include <algorithm>
    #include <functional>
    #include <type_traits>

    namespace util {
        struct sort_pointers {
            bool operator() ( int *a, int *b ) {
                return *a < *b;
            }
        };

        template <typename T, bool is_pointer = !std::tr1::is_pointer<T>::value>
        struct sort_helper {
            typedef std::less<T> wont_compare_pointers;
        };

        template <typename T>
        struct sort_helper<T,false> {
        };

        template <typename Iterator>
        void sort( Iterator start, Iterator end )
        {
            std::sort( start,
                       end,
                       sort_helper
                       <
                            typename Iterator::value_type
                       >::wont_compare_pointers() );
        }

        template <typename Iterator, class Func>
        void sort( Iterator start, Iterator end, Func f ) {
            std::sort( start, end, f );
        }
    }

    int main() {
        std::vector<int> v1;
        std::vector<int*> v2;
        srand(time(0));

        for( int i = 0; i < 10; ++i ) {
            v1.push_back(rand());
        }

        util::sort( v1.begin(), v1.end() );

        for( int i = 0; i < 10; ++i ) {
            v2.push_back(&v1[i]);
        }

        /* util::sort( v2.begin(), v2.end() ); */ //fails.
        util::sort( v2.begin(), v2.end(), util::sort_pointers() );

        return 0;
    }

std::tr1::is_pointer was just what it was called in Visual Studio 2008, but I think Boost has one too, and newer compiles might provide it as std::is_pointer. I'm sure someone would be able to write a prettier solution, but this appears to work.

But I must say, I agree with cogwheel, there is no reason for this, the programmer should be able to see if this is going to be a problem and act accordingly.

Addition:

You can generalize it a bit more I think, to automatically select a functor that will dereference the pointers and compare the values:

namespace util {
    template <typename T>
    struct sort_pointers {
        bool operator() ( T a, T b ) {
            return *a < *b;
        }
    };

    template <typename T, bool is_pointer = !std::tr1::is_pointer<T>::value>
    struct sort_helper {
        typedef std::less<T> compare;
    };

    template <typename T>
    struct sort_helper<T,false> {
        typedef sort_pointers<T> compare;
    };

    template <typename Iterator>
    void sort( Iterator start, Iterator end )
    {
        std::sort( start,
                   end,
                   sort_helper
                   <
                        typename Iterator::value_type
                   >::compare() );
    }
}

That way you don't have to think about if you're providing it with pointers to compare or not, it will automatically be sorted out.