current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 6 down vote favorite

Once you define the < operator, you can have an estimation of how the rest of relational operators behave. I'm trying to implement a way to do that for my classes.

What I want is to define only the < and the rest of the operators to be defaulted implicitly. What I've got so far is this design, which I'll elaborate on further down:

template<typename T>
struct relational
{
    friend bool operator> (T const &lhs, T const &rhs) { return rhs < lhs; }
    friend bool operator==(T const &lhs, T const &rhs) { return !(lhs < rhs || lhs > rhs); }
    friend bool operator!=(T const &lhs, T const &rhs) { return !(rhs == lhs); }
    friend bool operator<=(T const &lhs, T const &rhs) { return !(rhs < lhs); }
    friend bool operator>=(T const &lhs, T const &rhs) { return !(lhs < rhs); }
};

So for a class that implements the < operator it would just take inheriting from relational to have the rest of the operators defaulted. 

struct foo : relational<foo>
{ 
    // implement < operator here
};
  1. Are there any alternative, better designs?
  2. Is there a time bomb in this code?

  3. While trying to use this code with classes that already defined one of { >, ==, !=, <=, >= } I had ambiguity problems. The implementation I opted for (also the one existing in the linked Github repository) is tweaked like so 

    // inside the relational struct
friend bool operator>(relational const &lhs, relational const &rhs)
{ // functions that involve implicit conversion are less favourable in overload resolution
        return (T const&)rhs < (T const&)lhs; 
}

Are my problems over with this trick?

Here's a demo of the code working.

share|improve this question
2  
I unfortunately don't have much to say, but if you're wanting to see a well-established, fully fleshed out version of your concept, you could take a look at Boost.Operators: boost.org/doc/libs/1_55_0b1/libs/utility/operators.htm –  Corbin Apr 30 '14 at 22:11
2  
Another alternative is std::rel_ops. These are often criticized as "greedy and unfriendly" but I think this is only because they have only one template parameter. With two different parameters for lhs and rhs, I don't think there would be any ambiguities. –  iavr Apr 30 '14 at 22:36
    
Note: There is already an answer at stackoverflow.com/questions/23388739/… –  Dieter Lücking May 1 '14 at 6:14
    
@DieterLücking Check the edit log of this question. I was mentioning that I din't got enough input there, that's why I'm asking here. Then Jamal edited that bit out. –  Nikos Athanasiou May 1 '14 at 6:36
    
@NikosAthanasiou Now, I would down vote the edit of Jamal, if possible - sorry. –  Dieter Lücking May 1 '14 at 7:36

2 Answers 2

up vote 2 down vote accepted

First, you should seriously consider if you want keep you design (also in boost/operators.hpp) where a class inherits its operators from a base, or the design of std::rel_ops and Edwards's answer, where operators are defined as generic function templates. I think there are plus'es and minus'es, so there is no clear winner.

Assuming you want to keep your design, I find two issues:

  • It's better to design operators to match exactly rather than require conversions; otherwise, ambiguities and surprises will come sooner or later. One way to achieve this while being able to override the default behaviour is to separate interface from implementation.

  • This class makes things more convenient for the programmer, but officially accepts their laziness, at the cost of runtime performance. It is also possible to reduce code through implementation that is generic with respect to operators, at no performance loss.

Here is my suggestion, dealing with both issues, but staying as close as possible to your design:

template<typename T>
struct relational
{
    // cast to base/derived class
    static const relational&
    base(T const& a) { return static_cast <const relational&>(a); }

    const T& der() const { return static_cast<const T&>(*this); }

    // interface: relational operators
    friend bool operator< (T const &a, T const &b) { return base(a).compare(op::lt(), b); }
    friend bool operator> (T const &a, T const &b) { return base(a).compare(op::gt(), b); }
    friend bool operator==(T const &a, T const &b) { return base(a).compare(op::eq(), b); }
    friend bool operator!=(T const &a, T const &b) { return base(a).compare(op::ne(), b); }
    friend bool operator<=(T const &a, T const &b) { return base(a).compare(op::le(), b); }
    friend bool operator>=(T const &a, T const &b) { return base(a).compare(op::ge(), b); }

    // private dispatcher, to be called by relational operators
    template<typename F>
    bool compare(F f, T const &b) const { return der().comp(f, b); }

protected:
    // protected implementation, possibly overriden, to be called by compare
    bool comp(op::gt, T const &b) const { return b < der(); }
    bool comp(op::eq, T const &b) const { return !(der() < b || der() > b); }
    bool comp(op::ne, T const &b) const { return !(b == der()); }
    bool comp(op::le, T const &b) const { return !(b < der()); }
    bool comp(op::ge, T const &b) const { return !(der() < b); }
};

Here, operators are defined for T as in your original design and not for the base class as in your eventual workaround. When a derived class needs to define anything else than operator<, it overrides comp rather than defining a relational operator directly. comp is the implementation. operator@ is the interface.

Operators are represented by function objects like op::lt, op::gt etc. More on them below. Hence a derived class can override just a single overload of comp, or all of them with a template.

base and compare are not very important; they are only needed to make accessible to the operators what is accessible to relational.

Let's look at an example:

template<typename... T>
class direct : std::tuple<T...>, public relational<direct<T...>>
{
    friend relational<direct>;
    using U = std::tuple<T...>;
    const U& tup() const { return static_cast<const U&>(*this); }

    // override all functions
    template<typename F>
    bool comp(F f, const direct &b) const { return f(tup(), b.tup()); }

public:
    using U::U;
};

direct derives an std::tuple both for data storage and for providing lexicographical comparisons. It defines all comparison operators by overriding all overloads of comp with a single template.

A second example is like direct but reverses the relational operators:

template<typename... T>
class reverse : std::tuple<T...>, public relational<reverse<T...>>
{
    friend relational<reverse>;
    using relational<reverse>::comp;
    using U = std::tuple<T...>;
    const U& tup() const { return static_cast<const U&>(*this); }

    // override operator< only, rest is automatically generated
    bool comp(op::lt, const reverse &b) const { return tup() > b.tup(); }

public:
    using U::U;
};

It overrides only one overload of comp, the rest are as defined in the base class relational, but this comes at a runtime cost. I call it the lazy version. Why? Because with a little more typing, we could avoid the cost:

template<typename... T>
class reverse : std::tuple<T...>, relational<reverse<T...>>
{
    friend relational<reverse>;
    using U = std::tuple<T...>;
    const U& tup() const { return static_cast<const U&>(*this); }

    // override all functions (in fact, only == and !=)
    template<typename F>
    bool comp(F f, const reverse &b) const { return f(tup(), b.tup()); }

    // override remaining functions, each separately
    bool comp(op::lt, const reverse &b) const { return tup() >  b.tup(); }
    bool comp(op::gt, const reverse &b) const { return tup() <  b.tup(); }
    bool comp(op::le, const reverse &b) const { return tup() >= b.tup(); }
    bool comp(op::ge, const reverse &b) const { return tup() <= b.tup(); }

public:
    using U::U;
};

This overrides all overloads of comp, manually. Code is longer, but there is no runtime cost. It's better to do this if the cost of comparison is low. We might prefer the lazy version if the cost of high (e.g. lexicographical comparison with more the 10 elements), so our "laziness" penalty is proportionally small. One exception is operator== (also inherited by !=), which doubles the lexicographical comparison cost, so you'd better manually override == as well (then != will be fine). The point is: think about the cost of being lazy.

Here is how we would use the above examples:

direct <int,int> d1{5,2}, d2{6,1}, d3{6,5}, d4{6,5};
reverse<int,int> r1{5,2}, r2{6,1}, r3{6,5}, r4{6,5};

std::cout << (d1 > d2) << " " << (d2 >= d3) << " " << (d3 >= d4) << std::endl;  // 0 0 1
std::cout << (r1 > r2) << " " << (r2 >= r3) << " " << (r3 >= r4) << std::endl;  // 1 1 1

See also live example.

Now, the function objects are like std::less etc., but slightly different:

namespace op
{
    struct lt
    {
        template<typename A, typename B>
        auto operator()(A&& a, B&& b) const
        -> decltype(std::forward<A>(a) < std::forward<B>(b))
            { return std::forward<A>(a) < std::forward<B>(b); }
    };

    // similarly for gt, eq, ne, le, ge
}

These are as generic as possible, with perfect forwarding and automatically deduced return type (in case someone defines anything other than bool). So they can be used anywhere, not just for this solution. If you are feeling lazy, you could generate them with macros.

share|improve this answer
up vote 3 down vote

The problem I see is that it requires that the class be derived from relational, which is not a very durable design. Instead, why not use the fact that "substitution failure is not an error" (which has the regrettable but commonly used abbreviation SFINAE). the use of real functions is preferred over the instantiation of templates. That is, if an operator<= actually exists, the templated form will be ignored. That might look like this:

template<typename T>
bool operator> (T const &lhs, T const &rhs) { return !(lhs <= rhs); }
template<typename T>
bool operator< (T const &lhs, T const &rhs) { return !(lhs >= rhs); }
template<typename T>
bool operator==(T const &lhs, T const &rhs) { return lhs <= rhs && lhs >= rhs; }
template<typename T>
bool operator!=(T const &lhs, T const &rhs) { return !(rhs == lhs); }
template<typename T>
bool operator<=(T const &lhs, T const &rhs) { return lhs < rhs || (!(rhs < lhs)); }
template<typename T>
bool operator>=(T const &lhs, T const &rhs) { return lhs > rhs || (!(rhs > lhs)); }

Note that the differences are that I've made each comparison operator its own template and that I've added one for < and defined <= and >= in terms of < and > respectively.

The result is that if any of the comparisons <, <=, > or >= are defined, then all of the other comparisons are also available.

Edit: I should probably have pointed out why this is different from std::rel_ops. With std::rel_ops, in order to have all six relative operators, you must define operator< and operator==. With this scheme, however, as long as at least one of the ordering operators is defined, all others will also be available. This is due to the way these templates are deliberately circularly defined. That is, > is defined in terms of <= which is defined using < which is defined using >= which is defined using >. This arguably adds flexibility, but it also means that it's possible that the performance is not as good because a comparison may go through several templated function instantiations.

Further, if none of those operators is defined, that circular definition will mean infinite recursion at runtime until your stack is exhausted, putting this set of templates into the category of "perhaps more dangerous than clever" so Caveat lector.

share|improve this answer
1  
This is exactly the logic of std::rel_ops. You should definitely define those inside a namespace. Also, if you don't use two different template parameters for lhs, rhs, ambiguities are on the way and some people may hate you some time. Plus, I don't see any SFINAE here. –  iavr May 1 '14 at 10:23
    
It's substantively and deliberately different from the logic of std::rel_ops because unlike that, it's carefully designed not to care which of the four named relational operators is defined. They are circularly defined as { > , <=, <, >= } where each element is defined in terms of the next element while std::rel_ops insists on definitions of operators < and == only. This flexibility, however, is purchased with a potential performance hit. And you're right - it's not SFINAE, it's "real functions are preferred over template instantiation." I'll update the answer. –  Edward May 1 '14 at 12:14
1  
I meant the fact that they are generic function templates rather than concrete functions for the type you are defining, so that you don't need to derive anything. I didn't mean the exact computations. Performance is an important point indeed. We should be aware of the cost of our laziness. –  iavr May 1 '14 at 13:23
    
@iavr I think this design is wrong. And it takes only one argument to prove it. If this code exists in a code base, every class (that 'sees' the header) that defines < will have all other operations defined whether you want it or not. The concept of explicitly deriving from an empty base as relational is not a problem but a solution. std::rel_ops kind of bypasses this by requiring its namespace to be explicitly brought into scope, because that scope may have objects that should have nothing implicitly generated for them. –  Nikos Athanasiou May 5 '14 at 16:36
    
@NikosAthanasiou: I believe that was the point of iavr's first comment that these should be placed inside a namespace (and I would add ... if you use them at all). –  Edward May 5 '14 at 16:43

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.