38

I'm implementing variadic min/max functions. A goal is to take advantage of the compile time known number of arguments and perform an unrolled evaluation (avoid run-time loops). The current state of the code is as follows (presenting min - max is similar)

#include <iostream>  

using namespace std;

template<typename T>
T vmin(T val1, T val2)
{
    return val1 < val2 ? val1 : val2;
}

template<typename T, typename... Ts>
T vmin(T val1, T val2, Ts&&... vs)
{
    return val1 < val2 ?
        vmin(val1, std::forward<Ts>(vs)...) : 
            vmin(val2, std::forward<Ts>(vs)...);
}

int main()
{
    cout << vmin(3, 2, 1, 2, 5) << endl;    
    cout << vmin(3., 1.2, 1.3, 2., 5.2) << endl;
    return 0;
}

Now this works, but I have some questions / problems :

  1. The non variadic overload has to accept its arguments by value. If I try passing other types of ref I have the following results

    • universal references && -> compilation error
    • const references const& -> OK
    • plain references & -> compilation error

    Now I know that function templates mix weirdly with templates but is there any specific know-how for the mix up at hand ? What type of arguments should I opt for?

  2. Wouldn't the expansion of the parameter pack by sufficient ? Do I really need to forward my arguments to the recursive call ?

  3. Is this functionallity better implemented when wrapped inside a struct and exposed as a static member function. Would the ability to partial specialize buy me anything ?

  4. Is there a more robust/efficient implementation/design for the function version ? (particullarly I'm wondering whether a constexpr version would be a match for the efficiency of template metaprogramming)

Community
  • 1
  • 1
Nikos Athanasiou
  • 29,616
  • 15
  • 87
  • 153
  • Good question. You can even implement `min` like [this](http://stackoverflow.com/a/16498347/952747). – masoud May 22 '14 at 19:07
  • 6
    Why don't you use `std::min` with `std::initializer_list`, i.e. `template auto vmin(Args&&... args) { return std::min({ std::forward(args)... }); }`? – nosid May 22 '14 at 19:12
  • @nosid Ok valid point, but I'm trying (again it may not have much meaning) to implement a version that would perform compile time unrolling, as opposed to a function that runs a loop at runtime (maybe I should mention that in the question - I just noticed that it's not apparent) – Nikos Athanasiou May 22 '14 at 19:14
  • @nosid I think you should use something like `std::common_type` in your variant. – Constructor May 22 '14 at 19:23
  • 3
    Consider implementing along the lines of `vmin(vmin(val1, val2), vs...)`. –  May 22 '14 at 21:58

10 Answers10

30

live example

This does perfect forwarding on arguments. It relies on RVO for return values, as it returns a value type regardless of the input types, because common_type does that.

I implemented common_type deduction, allowing mixed types to be passed in, and the "expected" result type output.

We support the min of 1 element, because it makes the code slicker.

#include <utility>
#include <type_traits>

template<typename T>
T vmin(T&&t)
{
  return std::forward<T>(t);
}

template<typename T0, typename T1, typename... Ts>
typename std::common_type<
  T0, T1, Ts...
>::type vmin(T0&& val1, T1&& val2, Ts&&... vs)
{
  if (val2 < val1)
    return vmin(val2, std::forward<Ts>(vs)...);
  else
    return vmin(val1, std::forward<Ts>(vs)...);
}


int main()
{
  std::cout << vmin(3, 2, 0.9, 2, 5) << std::endl;

  std::cout << vmin(3., 1.2, 1.3, 2., 5.2) << std::endl;

  return 0;
}

Now, while the above is a perfectly acceptable solution, it isn't ideal.

The expression ((a<b)?a:b) = 7 is legal C++, but vmin( a, b ) = 7 is not, because std::common_type decays is arguments blindly (caused by what I consider an over-reaction to it returning rvalue references when fed two value-types in an older implementation of std::common_type).

Simply using decltype( true?a:b ) is tempting, but it both results in the rvalue reference problem, and does not support common_type specializations (as an example, std::chrono). So we both want to use common_type and do not want to use it.

Secondly, writing a min function that doesn't support unrelated pointers and does not let the user change the comparison function seems wrong.

So what follows is a more complex version of the above. live example:

#include <iostream>
#include <utility>
#include <type_traits>

namespace my_min {

  // a common_type that when fed lvalue references all of the same type, returns an lvalue reference all of the same type
  // however, it is smart enough to also understand common_type specializations.  This works around a quirk
  // in the standard, where (true?x:y) is an lvalue reference, while common_type< X, Y >::type is not.
  template<typename... Ts>
  struct my_common_type;

  template<typename T>
  struct my_common_type<T>{typedef T type;};

  template<typename T0, typename T1, typename... Ts>
  struct my_common_type<T0, T1, Ts...> {
    typedef typename std::common_type<T0, T1>::type std_type;
    // if the types are the same, don't change them, unlike what common_type does:
    typedef typename std::conditional< std::is_same< T0, T1 >::value,
      T0,
    std_type >::type working_type;
    // Careful!  We do NOT want to return an rvalue reference.  Just return T:
    typedef typename std::conditional<
      std::is_rvalue_reference< working_type >::value,
      typename std::decay< working_type >::type,
      working_type
    >::type common_type_for_first_two;
    // TODO: what about Base& and Derived&?  Returning a Base& might be the right thing to do.
    // on the other hand, that encourages silent slicing.  So maybe not.
    typedef typename my_common_type< common_type_for_first_two, Ts... >::type type;
  };
  template<typename... Ts>
  using my_common_type_t = typename my_common_type<Ts...>::type;
  // not that this returns a value type if t is an rvalue:
  template<typename Picker, typename T>
  T pick(Picker&& /*unused*/, T&&t)
  {
    return std::forward<T>(t);
  }
  // slight optimization would be to make Picker be forward-called at the actual 2-arg case, but I don't care:
  template<typename Picker, typename T0, typename T1, typename... Ts>
  my_common_type_t< T0, T1, Ts...> pick(Picker&& picker, T0&& val1, T1&& val2, Ts&&... vs)
  {
    // if picker doesn't prefer 2 over 1, use 1 -- stability!
    if (picker(val2, val1))
      return pick(std::forward<Picker>(pick), val2, std::forward<Ts>(vs)...);
    else
      return pick(std::forward<Picker>(pick), val1, std::forward<Ts>(vs)...);
  }

  // possibly replace with less<void> in C++1y?
  struct lesser {
    template<typename LHS, typename RHS>
    bool operator()( LHS&& lhs, RHS&& rhs ) const {
      return std::less< typename std::decay<my_common_type_t<LHS, RHS>>::type >()(
          std::forward<LHS>(lhs), std::forward<RHS>(rhs)
      );
    }
  };
  // simply forward to the picked_min function with a smart less than functor
  // note that we support unrelated pointers!
  template<typename... Ts>
  auto min( Ts&&... ts )->decltype( pick( lesser(), std::declval<Ts>()... ) )
  {
    return pick( lesser(), std::forward<Ts>(ts)... );
  }
}

int main()
{
  int x = 7;
  int y = 3;
  int z = -1;
  my_min::min(x, y, z) = 2;
  std::cout << x << "," << y << "," << z << "\n";
  std::cout << my_min::min(3, 2, 0.9, 2, 5) << std::endl;
  std::cout << my_min::min(3., 1.2, 1.3, 2., 5.2) << std::endl;
  return 0;
}

The downside to the above implementation is that most classes do not support operator=(T const&)&&=delete -- ie, they do not block rvalues from being assigned to, which can lead to surprises if one of the types in the min does not . Fundamental types do.

Which is a side note: start deleting your rvalue reference operator=s people.

Yakk - Adam Nevraumont
  • 262,606
  • 27
  • 330
  • 524
  • Hmm. Despite naive reading of `common_type` saying `common_type::type` is the type of `true?std::declval():std::declval()`, it appears that `common_type::type` is not `int&` but rather `int`. Strange. – Yakk - Adam Nevraumont May 22 '14 at 20:12
  • I think the base case should have a single argument, this also removes the need to separate `T2&& v0` from the parameter pack. See http://ideone.com/5J6BBw – Ben Voigt May 22 '14 at 20:15
  • Oh better yet, lose the `std::common_type` entirely. See http://ideone.com/OcQZ06 – Ben Voigt May 22 '14 at 20:20
  • @Yakk I think it is so because `common_type::type` can not be `float&`. – Constructor May 22 '14 at 20:22
  • @benvoigt that is C++1y. And, in theory, `common_type` does things `?` cannot do. More fixes inc. – Yakk - Adam Nevraumont May 22 '14 at 20:26
  • Now uses `if`. Passing in `std::chrono` types will show a difference, as `?` does not know `common_type` specializations. – Yakk - Adam Nevraumont May 22 '14 at 20:29
  • @constructor and in that case it should return `float` or `double`. Need to standard delve, then fix cpprefernce docks, as they are in error, or ideone compiler is, or both. – Yakk - Adam Nevraumont May 22 '14 at 20:31
  • @Yakk As I can see the key is in the line `typedef decltype(true ? declval() : declval()) type;` in the implementation of `std::common_type` for two arguments. To be exact the cause is in the behavior of `?` operator. – Constructor May 22 '14 at 20:46
  • @Constructor as demonstrated [here](https://ideone.com/FvvmvC), `common_type::type` is not the same as `decltype( true?std::declval():std::declval())` (replacing `y_t x` with `x_t x` makes it fail to compile). Maybe different version of `common_type`? – Yakk - Adam Nevraumont May 22 '14 at 20:52
  • @Constructor [bah](http://stackoverflow.com/questions/21975812/should-stdcommon-type-use-stddecay), a `decay` was added, so there is no way to make a high quality `min` that works like chained `?` and supports `common_type` specializations. – Yakk - Adam Nevraumont May 22 '14 at 20:56
  • @Yakk Yes, I have already [noticed](http://ideone.com/rOPEa2) that they are not the same. I think it is very strange. – Constructor May 22 '14 at 21:03
  • @Yakk Yes, it is an interesting and sad reference. But what do you mean by "a high quality `min`" ? A possibility to assign a value to a result of `min` call? I think it is not so important. But adding `std::decay_t` to `std::common_type` is rather unexpected and unpleasant change. – Constructor May 22 '14 at 21:14
  • @NikosAthanasiou No, it is not the cause. See the [reference](http://stackoverflow.com/a/21976059/3043539) which is given by Yakk. – Constructor May 22 '14 at 21:16
  • 6
    Stepanov argues that `min(a, b)` should return `a` iff `a <= b`, that is, `b < a ? b : a`. Has to do with "natural order" and is related to stability. Somewhere in this series: http://www.youtube.com/watch?v=aIHAEYyoTUc – dyp May 22 '14 at 23:05
  • @dyp good point, one more edit toward community wikification! I should write a better `common_type` and use it as well, because `min(a,b)=c` is just too much fun to write. (sadly, rarity of `operator=(...)&&=delete` probably makes it dangerous) – Yakk - Adam Nevraumont May 23 '14 at 01:16
  • You have a typo, the first branch of the `if` ends with a colon instead of semicolon. – Blastfurnace May 23 '14 at 01:17
  • The semantic of `vmin` should be the same for the 1 argument version and for more arguments. But if you pass one lvalue `a` to `vmin` then the expression `vmin(a)` is an lvalue while in all other cases it is a prvalue. Solution: change the return type to `typename std::remove_reference::type`. – MWid May 23 '14 at 13:56
  • @MWid actually, I intend to change it to being an lvalue reference for many types, if the types are all lvalue reference and match. The goal is to match the semantics of `?` extended with `common_type` support, and to eliminate needless copies. Plus add support for user-defined binary comparators/pickers. – Yakk - Adam Nevraumont May 23 '14 at 14:30
  • @Blastfurnace fixed your typo when I added another implementation. – Yakk - Adam Nevraumont May 23 '14 at 14:58
  • I think you can still break `my_common_type` with malicious specializations of `common_type::type != T0`; also `struct X { operator int&(); }; my_common_type::type` will not return an lvalue ref as far as I can see. I'm not sure if I didn't want an rvalue ref for `x = min(func1(), func2())`. IIRC you said `min(func1(), func2()) = x;` is dangerous if the first one is an rvalue? Isn't that the problem of the user? (`std::common_type` is broken) – dyp May 23 '14 at 21:35
  • @dyp Such a specialization would ignore the malicious specialization of `std::common_type`, so not sure if that is a 'break'. Your `X` is interesting, hmm. Returning rvalue refs is usually wrong outside of casts (like `move` or `forward`), as reference lifetime extension relies on returning values in that case. The danger is with `common_type_t=C` if `struct A {}; struct C{ C(C const&)=default; C(A const&):x(0); int x=2; bool operator<(C const& o)const{return x – Yakk - Adam Nevraumont May 23 '14 at 21:43
  • @dyp aha: check if `U` and `T` can be converted to `std_type&`? Maybe also that they are lvalues? That might catch thise corner cases: spot any flaws? – Yakk - Adam Nevraumont May 25 '14 at 04:41
  • I think we cannot know whether we're using a specialization of `common_type` or the general case. Some class `derived` can be converted to `base&`, but a specialization might want to create a new object: `common_type::type == base` – dyp May 25 '14 at 12:44
  • @Yakk-AdamNevraumont Wouldn't using an `auto&&` return type and let the compiler perfectly forward the arguments solve most problems mentioned here? I've seen the approached mentioned in [my answer](https://stackoverflow.com/a/73290633/4224575) used w/o problems so far. Unless I'm missing something? – Lorah Attkins Aug 09 '22 at 10:55
  • @LorahAttkins No, `auto&&` never causes temporary lifetime extension in return values. – Yakk - Adam Nevraumont Aug 09 '22 at 17:21
  • @LorahAttkins Or to be more explicit, sometimes min of non-uniform types requires conversion, which requires storage. `auto&&` does not permit storage. So you must have a dangling return value in that case (or something equally ugly, like global/thread local storage for it). – Yakk - Adam Nevraumont Aug 09 '22 at 17:41
18

I appreciate the thought Yakk put into return types so I wouldn't have to, but it gets a lot simpler:

template<typename T>
T&& vmin(T&& val)
{
    return std::forward<T>(val);
}

template<typename T0, typename T1, typename... Ts>
auto vmin(T0&& val1, T1&& val2, Ts&&... vs)
{
    return (val1 < val2) ?
      vmin(val1, std::forward<Ts>(vs)...) :
      vmin(val2, std::forward<Ts>(vs)...);
}

Return type deduction is pretty awesome (may require C++14).

Ben Voigt
  • 277,958
  • 43
  • 419
  • 720
  • 1
    *"may require C++14"* It *does* require C++1y. I wonder if `decltype(auto)` might be a better choice here. Also, http://stackoverflow.com/questions/23815138/implementing-variadic-min-max-functions#comment36643409_23816018 – dyp May 22 '14 at 23:08
  • Pass it two `std::chrono` types that differ: `common_type` is smarter than `?` – Yakk - Adam Nevraumont May 22 '14 at 23:20
  • @Yakk: But which result is "better"? Is it necessary that `vmin` work on types for which `?:` does not? – Ben Voigt May 22 '14 at 23:33
  • @BenVoigt: A question, what is the advantageous of `&&` in this case rather than `const &` while the `min` isn't copying or modifying the inputs? Isn't it better to pass the inputs by `const &` (which doesn't need to perfect-forwarding) as same as [this](http://stackoverflow.com/questions/16497977/general-min-and-max-c/16498347#16498347)? – masoud May 23 '14 at 08:32
  • 1
    @MM. As I said in a comment to your question, the arguments are copied/moved. The `auto` return type deduction, like template type deduction, never deduces a reference, hence this function returns by value. (Therefore my suggestion to use `decltype(auto)`.) – dyp May 23 '14 at 10:10
  • @dyp: My question is about entry parameters but I think your comment is talking about returning. – masoud May 23 '14 at 10:13
  • @MM. Well if your parameter is a `const&`, you can only copy into the return value. If it's a universal reference, you can copy and move into the return value, depending on the argument expression (call site). – dyp May 23 '14 at 13:45
  • @benvoigt being able to say "the least of all of these `std::chrono` time points seems like better than the alternative, and `std::chrono` specializes `std::common_type` to tell what is the proper type for this kind of operation. `?:` won't work. And barring specialization, `std::common_type::type` is `std::decay::type`, which matches your `auto`-decay. – Yakk - Adam Nevraumont May 23 '14 at 15:06
  • @dyp Doesn't `decltype(auto)` results in UB if you pass in `double` and `int`, as the output is a `double&&` possibly converted from the `int`, and that `double&&` lifetime ends before the caller gets at it? Or is there magic I'm missing? – Yakk - Adam Nevraumont May 23 '14 at 15:07
  • @Yakk True. There are no prvalues with universal refs. `decltype(auto)` is useful for lvalue references, so maybe you can fix it by explicitly decaying xvalues o.O – dyp May 23 '14 at 16:51
11

There is a solution in C++17 which beats all answers proposed so far:

template <typename Head0, typename Head1, typename... Tail>
constexpr auto min(const Head0 &head0, const Head1 &head1, const Tail &... tail)
{
    if constexpr (sizeof...(tail) == 0) {
        return head0 < head1 ? head0 : head1;
    }
    else {
        return min(min(head0, head1), tail...);
    }
}

Notice how this:

  • requires only one function
  • you can't call this with fewer than two parameters
  • it compiles optimally

Using gcc 10.2 with -O3, the accepted answer compiles to:

min(int, int, int):
        cmp     esi, edi
        jge     .L2
        cmp     esi, edx
        mov     eax, edx
        cmovle  eax, esi
        ret
.L2:
        cmp     edi, edx
        mov     eax, edx
        cmovle  eax, edi
        ret

There are more instructions and a conditional jump for whatever reason. My solution compiles only to:

min(int, int, int):
        cmp     esi, edx
        mov     eax, edi
        cmovg   esi, edx
        cmp     esi, edi
        cmovle  eax, esi
        ret

This is identical to just calling std::min recursively for three parameters. (see https://godbolt.org/z/snavK5)

Jan Schultke
  • 17,446
  • 6
  • 47
  • 96
  • 1
    You should `std::forward(tail)...`. – darkdragon Sep 20 '22 at 09:48
  • @darkdragon thanks for the suggestion. Using `&&` without forwarding was wrong, but we also cannot use `std::forward` for `Head0` and `Head1` because they're used twice, and if the less-than operator is overloaded with rvalue-qualification, returning a value may be use-after-move. This is why I've opted for using `const&` everywhere instead. – Jan Schultke Jun 11 '23 at 22:49
5

4) Here is one possible way to implement a constexpr version of this function:

#include <iostream>
#include <type_traits>

template <typename Arg1, typename Arg2>
constexpr typename std::common_type<Arg1, Arg2>::type vmin(Arg1&& arg1, Arg2&& arg2)
{
    return arg1 < arg2 ? std::forward<Arg1>(arg1) : std::forward<Arg2>(arg2);
}

template <typename Arg, typename... Args>
constexpr typename std::common_type<Arg, Args...>::type vmin(Arg&& arg, Args&&... args)
{
    return vmin(std::forward<Arg>(arg), vmin(std::forward<Args>(args)...));
}

int main()
{
    std::cout << vmin(3, 2, 1, 2, 5) << std::endl;
    std::cout << vmin(3., 1.2, 1.3, 2., 5.2) << std::endl;
}

See live example.

Edit: As @Yakk noted in comments the code std::forward<Arg1>(arg1) < std::forward<Arg2>(arg2) ? std::forward<Arg1>(arg1) : std::forward<Arg2>(arg2) may cause problems in some situations. arg1 < arg2 ? std::forward<Arg1>(arg1) : std::forward<Arg2>(arg2) is more appropriate variant in this case.

Community
  • 1
  • 1
Constructor
  • 7,273
  • 2
  • 24
  • 66
  • You `forward` the same argument twice along some execution paths (and not a chain). This is a bad idea. If an rvalue overload for `<` is destructive, you'll return nonsense. – Yakk - Adam Nevraumont May 22 '14 at 19:59
  • @Yakk Oh, I think you are right. Can I steal the solution of this problem from your answer to fix my one? – Constructor May 22 '14 at 20:04
3

You cannot bind a temporary to a non-const reference, that is why you probably get the compilation error. That is, in vmin(3, 2, 1, 2, 5), the parameters are temporaries. It will work if you declare them as for example int first=3,second=2 and so on, then invoke vmin(first,second...)

vsoftco
  • 55,410
  • 12
  • 139
  • 252
3

With c++17and not using recursion:

template <typename T, T ... vals>
constexpr T get_max(std::integer_sequence<T, vals...> = std::integer_sequence<T, vals...>())
{
     T arr[sizeof...(vals)]{vals...},
         max = 0;
     for (size_t i = 0; i != sizeof...(vals); ++i)
            max = arr[i] > max ? max = arr[i] : max;
     return max;
}

Function can be called by providing either template parameters or integer sequence as argument

get_max<int, 4, 8, 15, 16, 23, -42>();

using seq = std::integer_sequence<int, ...>;
get_max(seq());
Sergey Kolesnik
  • 3,009
  • 1
  • 8
  • 28
  • How I can calculate max type in case I have only type parameters, and want type is also be a result. Try this way but seems with only type sequence it is not working: template struct max { using seq = std::integer_sequence; using type = decltype( get_max( seq() ) ); }; – Sergey Inozemcev Apr 11 '21 at 22:41
  • @SergeyInozemcev `using seq = std::integer_sequence` makes no sence and won't compile in the first: you are trying to instantiate a template specialization with variadic template arguments `Ts...`. But `std::integer_sequense` only accepts non-template parameters. Looking at your code I guess you don' need to use `decltype` but `std::common_type` if you have an array of types. – Sergey Kolesnik Apr 12 '21 at 05:53
2

C++20 version

The solution presented by @Yakk-AdamNevraumont is good, it covers very well aspects of lvalue and rvalue, not allowing to return a reference to a temporary, and yet returning lvalue reference if you can.

But the solution can now be modernized for C++20 and become much more concise and elegant:

template<typename... Ts>
struct common_return {
    using type = std::common_reference_t<Ts...>;
};

template<typename T, typename... Ts>
    requires std::is_lvalue_reference_v<T> &&
            (std::is_lvalue_reference_v<Ts> && ...)
            && ( std::same_as<T, Ts> && ... )
struct common_return<T, Ts...> {
    using type = std::common_reference_t<T, Ts...>&;
};

template<typename... Ts>
using common_return_t = typename common_return<Ts...>::type;

namespace my_min {
    template<typename T>
    T min(T&& t) {
        return std::forward<T>(t);
    }

    template<typename T1, typename T2, typename... Ts>
    common_return_t<T1, T2, Ts...> min(T1&& t1, T2&& t2, Ts&&... ts) {
        if(t2 > t1) {
            return min(std::forward<T1>(t1), std::forward<Ts>(ts)...);
        }
        return min(std::forward<T2>(t2), std::forward<Ts>(ts)...);
    }
}
Amir Kirsh
  • 12,564
  • 41
  • 74
1

Solution with a lambda and set

auto max = [](auto&& e1, auto&& e2, auto&&... args)
{
    return *std::set<typename std::decay_t<decltype(e1)>>{e1,e2,args...}.rbegin();
};
formigoni
  • 43
  • 5
0

With C++11, this solution should be fine ( with using std::max / std::min) :

#include <algorithm>

template<typename T>
T Max(T arg)
{
return arg;
}
template<typename T, typename Ts>
T Max(T arg, Ts... args)
{
return std::max(arg, Max(args...));
}

The performance is not so different from above solutions.

(It is checked via Microsoft VS 2019 / no optimization, using chrono library)

  • Calling the function with single element is valid.
0

Another approach is to leverage an auto&& return type and perfectly forward your results:

template <class T, class... Ts>
auto&& Min(T&& arg1, Ts&&... args)
{
    if constexpr (sizeof...(Ts))
    {
        auto &&rmin = Min(std::forward<Ts>(args)...);
        return arg1 < rmin ? std::forward<T>(arg1) : rmin;
    }
    else
    {
        return std::forward<T>(arg1);
    }
}

Demo

Lorah Attkins
  • 5,331
  • 3
  • 29
  • 63