Min function accepting varying number of arguments in C++17

Question

Come across this problem once again in the book The Modern C++ Challenge.

18. Minimum function with any number of arguments

Write a function template that can take any number of arguments and returns the minimum value of them all, using operator < for comparison. Write a variant of this function template that can be parameterized with a binary comparison function to use instead of operator <.

I wonder how simple and elegant the implementation could be using c++17. The following is the parameterized version. Ideas?

#include <algorithm>

template <typename Less, typename T, typename... Ts>
constexpr const T& min(Less less, const T& a, const T& b, const Ts&... rems) {
  if constexpr (sizeof...(rems)) {
    return min(less, std::min(a, b, less), rems...);
  }
  else {
    return std::min(a, b, less);
  }
}

Answer 1

template <typename Less, typename T, typename... Ts>
constexpr const T& min(Less less, const T& a, const T& b, const Ts&... rems) {

This function requires a minimum of 2 elements. A minimum element will exist if the user provides a single argument variadic list. Consider handling that.

auto& min1 = min(std::less<>{}, 4, 5);  // okay!
auto& min2 = min(std::less<>{}, 4);     // candidate function not viable, reqs 3 args, 2 provided

---

        return min(less, std::min(a, b, less), rems...);

Your approach here uses recursion and implementations are permitted to put a recursion depth limit on constexpr calculations. Consider an iterative solution that expands the pack while calculating the minimum.

template <typename Comparator, typename First, typename... Rest>
constexpr decltype(auto) variadic_min(Comparator cmp, First const& first, Rest const&... rest) {
    const First* result = std::addressof(first);

    // cast each evaluated expr to void in case of overloaded comma operator shenanigans
    ((void)(result = std::addressof(std::min(*result, rest, cmp))), ...);

    return *result;
}

An explanation with what is going on with the fold expression:

((void)(result = std::addressof(std::min(*result, rest, cmp))), ...);
 ^     ^         ^                                            ^ 
 |     |         |                        expand using comma op
 |     |         safer than built-in, now constexpr in 17
 |     evaluate the expression for each pack variable
 cast to void the entire expression to avoid overloaded comma op.

---

Just thinking beyond the standard library and reinventing the wheel. This min works fine for homogenous packs. What about heterogenous packs?

auto& min1 = min(std::less<>{}, 4, 5, -1);  // min1 = -1
auto& min2 = min(std::less<>{}, 4, 5, -1.); // candidate template ignored...

One of the benefits of the conditional operator (?:) is that if both resulting expressions return lvalues of the same type, then the result will have the same type. If that type is T&, we could assign to that variable. Could we mimic that behavior with min and friends?

auto a = 4;
auto b = 5;
((b < a) ? b : a)  = 42;        // a = 42, b = 5
min(std::less<>{}, a, b) = 42;  // cannot assign to return value, returns const-qualified type
std::min(a, b) = 42;            // same with standard library.
min(std::less<>{}, 5, 6) = 42   // cannot assign, makes sense!

Answer 2

This looks nice! Two issues I see here,

1. When compiling your template with clang, it refuses the if constexpr dispatch for every recursive instantiation with sizeof...(rems) > 1, e.g.

   error: constexpr if condition evaluates to 2, which cannot be narrowed to type bool [-Wc++11-narrowing]

   gcc seems to accept this, but the fix is quite simple, just be more explicit about it:

   if constexpr (sizeof...(rems) > 0)
   

2. Never underestimate the standard library. You are doing more work than necessary, have a look at overload #4 in the std::min signature. You can expand the parameter pack into a std::initializer_list and pass this to std::min, which simplifies your template and avoids its recursive instantiation. Additionally wrapping the arguments into a std::reference_wrapper ships around unnecessary copies.

   #include <functional>
   
   template <typename Less, typename... Ts>
   constexpr decltype(auto) min(Less less, const Ts&... rems) {
       return std::min({std::cref(rems)...}, less).get();
   }
   

   The return type of the std::min invocation is a std::reference_wrapper, to get around this, its get() member function is called.

   Of course, you will pay for the construction of sizeof...(rems) std::reference_wrapper instances and for the std::initializer_list.

Answer 3

It works well, according to my simple test program:

#include <functional>
int main()
{
    return min(std::less<int>(), 2, 0, 3);
}

I suggest that when you present code for review, you include the tests, too - I imagine that your testing is much more thorough than mine, and I'm too lazy to re-create the full suite. It also helps reviewers identify scenarios that are missing from the tests (or, conversely, scenarios that are redundant).

---

I recommend using perfect forwarding for the comparator:

constexpr const T& min(Less&& less,
//                         ^^

    return std::min(a, b, std::forward<Less>(less));
//                        ^^^^^^^^^^^^^^^^^^

There's a couple of other uses that need to be forwarded, too.

---

As lubgr mentioned, it's worth using an initializer list. If we want to avoid copying (if the inputs are large, or can't be copied), then we'll want to use a initializer-list of reference wrappers, and use std::min_element() (since the initializer-list std::min() returns a copy). That can be achieved like this:

#include <algorithm>
#include <functional>

template<typename Less, typename... T>
constexpr auto& min(Less&& less, const T& ...values)
{
    auto const compare_wrapped =
        [&less](auto const&a, auto const& b) {
            return std::forward<Less>(less)(a.get(), b.get());
        };

    auto const list = { std::ref(values)..., };
    auto const smallest =
        std::min_element(list.begin(), list.end(), compare_wrapped);
    return smallest->get();
}

int main()
{
    auto const a = 3;
    auto const b = 0;
    auto const c = 2;

    auto const& lowest = min(std::less<int>(), a, b, c);

    return &lowest != &b;
}

Or, more succinctly:

template<typename Less, typename... T>
constexpr auto& min(Less&& less, const T& ...values)
{
    return std::min({std::cref(values)...,}, std::forward<Less>(less)).get();
}

One defect in this implementation is that it will accept xvalues and happily return a (useless) reference in that case. I think it ought to be possible to distinguish that case, and forward the chosen xvalue as result, but I haven't had time to implement that.