CODEWITHSUNDEEP
c++c++-concepts
Alexandru Ica
Alexandru Ica

Reputation: 139

Is this max function decent?

I am trying to understand new c++ features and I came up with my implementation of max function:

#include <utility>

template <typename T>
concept bool Comparable() {
    return requires(T a, T b) 
    {
        {a > b} -> bool;
        {a < b} -> bool;
        {a >= b} -> bool;
        {a <= b} -> bool;
    };
}

template<Comparable T, Comparable U>
constexpr decltype(auto) max(T&& a, U&& b)
{
    return std::forward<T>(a) > std::forward<U>(b) ? std::forward<T>(a) : std::forward<U>(b);
}

int main()
{
    constexpr int a = 2, b = 3;

    return max(a, b);
}

Can this replace the macro version?

#define MAX(a, b) (((a) > (b)) ? (a) : (b))

And if not what can be improved?

I am compiling with gcc-8.1.0 and -std=c++2a, -fconcepts and -O3 flags

EDIT:

Thank you for your suggestions, this is how it looks now:

#include <utility>

template<typename T, typename U>
concept bool TypesLessComparable() {
    return requires(T a, U b)
    {
        {a < b} -> bool;
    };
}

template<typename T, typename U>
constexpr decltype(auto) max(T&& a, U&& b) requires (TypesLessComparable<T, U>() == true)
{
    return a < b ? std::forward<U>(b) : std::forward<T>(a);
}

int main()
{
    constexpr int a = 2, b = 3;

    return max(a, b);
}

Upvotes: 0

Views: 98

Answers (2)

Slava
Slava

Reputation: 44278

Is this max function decent?

No, there are at least 3 issues:

1 Unclear why you request multiple comparison operations when your function only requires one of them (it should be fine for a class only to define operator> when only max() is used)

2 Concept Comparable requires that type has comparison operations on itself, you may compare 2 object of different types. This invalidates usage of concepts (a function should use only what required/provided by concepts)

3 You may return objects of different types as well, one may convert to another but you should cover that by another concept.

Here is code example for case 2 and 3:

 struct A { friend bool operator>( const A&, const A& ); };
 struct B { friend bool operator>( const B&, const B& ); };

 auto x = max( A(), B() );

To summarize:

  • Your function should require minimum set of operations on type(s), necessary for it to operate.

  • If type(s) passed concepts validation your function should compile successfully.

This is most probably not full set of requirements.

Upvotes: 3

eerorika
eerorika

Reputation: 238461

std::forward<T>(a) > std::forward<U>(b) ? std::forward<T>(a) : std::forward<U>(b);

You forward the result twice. This is wrong for most types (only OK for types that implement move as a deep copy). You should remove forward from the comparison.

Upvotes: 3

Related Questions