2

I am writing a template function, with template argument X. Inside that function, I want to create std::set<std::pair<int, X>> such that:

  • objects in that set are sorted by the first (int) field of the pair (I don't care about how ties are broken)
  • I can add multiple objects with the same .first to the set, as long as their .second are not identical

If I knew that template argument X always has < defined, the most basic std::set<std::pair<int, X> (with the default comparator) would have worked perfectly fine. Unfortunately, I cannot assume anything about X.

I was thinking to "cheat" and use pointer-based comparison for the X field:

template <typename X>
struct DistCmp {
    bool operator()(const std::pair<int, X>& lhs, const std::pair<int, X>& rhs) const {
        return lhs.first < rhs.first || lhs.first == rhs.first && &lhs.second < &rhs.second;
    }
};

template <typename X>
void f() {
    std::set<std::pair<int, X>, DistCmp<X>> s{};
    // ...
}

(After all, I don't really care how the .second is compared, as long as it's not compared equally for non-identical objects.)

Unfortunately, I don't think it's correct. Partly because of the quote from C++ standard here (it suggests that pointer comparison, in general, is unspecified, so I can't rely on them being unequal for non-identical objects). And partly I just feel it's suspicious/hacky.

Is there any clean/portable solution for this issue?

Update:

One approach I thought about was to use == for the comparison of pointers instead of <. However, this is no good because it will cause pair<1, x> < pair<1, y> && pair<1, y> < pair<1, x> to be true. This violates the requirement of strict weak ordering and may cause stuff to break.

max
  • 49,282
  • 56
  • 208
  • 355
  • Is your question because you're concerned about things like X being a virtual member function? – xaxxon Jul 07 '17 at 06:05
  • @xaxxon actually, if it helps, I can exclude virtual inheritance completely. I'm just concerned that comparing pointers for inequality isn't the right idiom and so weird things can happen. Let's say X is something simple, like a pair of ints, or a struct I defined, etc. – max Jul 07 '17 at 06:08
  • If two `T*` pointers are different, then they refer to different objects. Also, I said virtual member functions, not virtual inheritance. " if either is a pointer to a virtual member function, the result is unspecified" That was the type that was called out in the link you put in your question. Which specific part of the content at that link made you wary of using pointer comparison? – xaxxon Jul 07 '17 at 06:12
  • Can you hash `X` and use that as an ordering? Judging from the fact that `X` can be equality compared, I'd assume it can be hashed. And by then you could just use `std::set>` – Passer By Jul 07 '17 at 06:38
  • Generating an ordering from an equivalence relation is mathematically impossible, so you'd need some more information one way or another – Passer By Jul 07 '17 at 06:46
  • @xaxxon I guess I was referring to this: `If two pointers `p` and `q` of the same type point to different objects that are not members of the same object or elements of the same array [...], the result of `p – max Jul 07 '17 at 08:01
  • @max that's why you should equality instead of relational when you just want to know if they're the same or not. That bit you quoted doesn't apply to operator==, only things like < and > – xaxxon Jul 07 '17 at 08:37
  • @xaxxon Sadly, you can't use `operator ==` . See my question update. – max Jul 07 '17 at 14:36

2 Answers2

-1

Short answer: use std::multiset and sidestep whole issue - multiset allows multiple keys, so just compare on first element of pair.

Note, that if you want to disallow the same VALUE of X in your map, you've to add requirement to X, so it were at least EqualityComparable. Otherwise you can't even detect, when values of X are the same and when are different.

Longer answer: Your code won't produce the result you're hoping for. Consider adding new pair <0, x> to map with <0, x>. std::map will try to find existence of <0, x> using temporary copy of <0, x>. It will use address of x (temporary!), it will compare false to everything, that is inside in map and std::map will find a place to insert based on address of x (temporary!). Then it will COPY x, thus changing address and possibly breaking its own ordering.

Radosław Cybulski
  • 2,952
  • 10
  • 21
  • it's not clear to me that unspecified behavior on operator== is a useful behavior. Couldn't it just always return true? or always return false? – xaxxon Jul 07 '17 at 06:20
  • Quoting: Two pointers of the same type compare equal if and only if they are both null, both point to the same function, or both represent the same address. You're comparing two pointers to X (which is fine), so you'll get true if and only if they point to the same address. – Radosław Cybulski Jul 07 '17 at 06:22
  • From the link he provided: "if either is a pointer to a virtual member function, the result is unspecified." – xaxxon Jul 07 '17 at 06:23
  • You arent comparing pointer to virtual member function. You're comparing pointer to pointer to virtual member function. (if X is pointer to virtual member function, then compare against &X). So you'll always compare against pointer to object. – Radosław Cybulski Jul 07 '17 at 06:29
  • two pointers to pointers may be different but point to the same object. You're not comparing X's, you're comparing X*'s. – xaxxon Jul 07 '17 at 06:33
  • Yes, they might. But your code clearly compares X* against each other and you asked is it fine, which i wrote, than yes, it's fine (as long as u dont care for result, only for consistency). If you want to compare X by value and you know operator == exists - use ordered_multimap and tell user to implement std::hash for X. – Radosław Cybulski Jul 07 '17 at 06:35
  • Unfortunately, I can't use `multiset` because `.erase` from `multiset` will delete all the items that evaluate the same; in my case, it would mean that it would delete items even if they have different `.second` - it would break the semantics I need. And yet, your explanation about why pointer comparison doesn't work seems correct (although my knowledge of C++ is very weak). So I suppose you're right that I must at least require that `X` is equality comparable. Still, even with this requirement met, I don't see what to do. – max Jul 07 '17 at 14:46
  • You can erase by iterator position (see http://en.cppreference.com/w/cpp/container/multiset/erase ). In other words you'd have to use lower_bound to find first element with given "key" (integer), then iterate using `++` until you find correct value of X using `operator ==` for comparision. – Radosław Cybulski Jul 09 '17 at 19:02
-1

I think ::std::map< int, ::std::set< X > > satisfies both of your criteria.

Update: since there might be no < operator to compare X instance you may want to write a template with specialization:

#include <map>
#include <set>
#include <vector>
#include <type_traits>
#include <algorithm>
#include <utility>

//  Default implementation is used when there is no < operator to compare T instances,
//  but there is == operator.
template< typename T, typename TDummy = void >
t_FancyContainerHelper final
{
    public: using
    t_Values = ::std::vector< T >;

    public: using
    t_IntToValues = ::std::map< int, t_Values >;

    public: static void
    Insert(t_IntToValues & int_to_values, int const key, T const & value)
    {
        auto p_pair{int_to_values.find(key)};
        if(int_to_values.end() != p_pair)
        (
            auto const & values(p_pair->second);
            if(values.end() != ::std::find(values.begin(), values.end(), value))
            {
                return;
            }
        }
        else
        {
            p_pair = int_to_values.emplace
            (
                ::std::piecewise_construct
            ,   ::std::forward_as_tuple(key)
            ,   ::std::forward_as_tuple()
            ).second;
        }
        auto & values(p_pair->second);
        values.emplace(value);
    }
};

//  This specialization is used when there is < operator to compare T instances.
template< typename T > class
t_FancyContainerHelper
<
    T
,   ::std::enable_if_t
    <
        ::std::is_same
        <
            bool
        ,   decltype
            (
                ::std::declval< T const & >()
                <
                ::std::declval< T const & >()
            )
        >
    >
> final
{
    public: using
    t_Values = ::std::set< T >;

    public: using
    t_IntToValues = ::std::map< int, t_Values >;

    public: static void
    Insert(t_IntToValues & int_to_values, int const key, T const & value)
    {
        auto p_pair{int_to_values.find(key)};
        if(int_to_values.end() != p_pair)
        (
            auto const & values(p_pair->second);
            if(values.end() != values.find(value))
            {
                return;
            }
        }
        else
        {
            p_pair = int_to_values.emplace
            (
                ::std::piecewise_construct
            ,   ::std::forward_as_tuple(key)
            ,   ::std::forward_as_tuple()
            ).second;
        }
        auto & values(p_pair->second);
        values.emplace(value);
    }
};
user7860670
  • 35,849
  • 4
  • 58
  • 84
  • X doesnt have operator <, so std::set might not compile. – Radosław Cybulski Jul 07 '17 at 06:41
  • Well then you can use `::std::map< int, ::std::vector< X > >` and perform lookup for X before inserting using `::std::find`. Though it will be slower. If you are writing a template then you may want to write specializations for both situations and use set or vector depending on whether X does or does not have `<` operator. – user7860670 Jul 07 '17 at 06:44
  • Yes, but your answer is different, can you please change it? It's misleading. ;) Also if you want to use SET of vectors (OP requested set not map), then multiset as i suggested is better. – Radosław Cybulski Jul 07 '17 at 06:57