structured bindings with std::minmax and rvalues

Question

I ran into a rather subtle bug when using std::minmax with structured bindings. It appears that passed rvalues will not always be copied as one might expect. Originally I was using a T operator[]() const on a custom container, but it seems to be the same with a literal integer.

#include <algorithm>
#include <cstdio>
#include <tuple>

int main()
{
    auto [amin, amax] = std::minmax(3, 6);
    printf("%d,%d\n", amin, amax); // undefined,undefined

    int bmin, bmax;
    std::tie(bmin, bmax) = std::minmax(3, 6);
    printf("%d,%d\n", bmin, bmax); // 3,6
}

Using GCC 8.1.1 with -O1 -Wuninitialized will result in 0,0 being printed as first line and:

warning: ‘<anonymous>’ is used uninitialized in this function [-Wuninitialized]

Clang 6.0.1 at -O2 will also give a wrong first result with no warning.

At -O0 GCC gives a correct result and no warning. For clang the result appears to be correct at -O1 or -O0.

Should not the first and second line be equivalent in the sense that the rvalue is still valid for being copied?

Also, why does this depend on the optimization level? Particularly I was surprised that GCC issues no warning.

Answer 1

There are two fundamental issues going on here:

1. min, max, and minmax for historic reasons return references. So if you pass in a temporary, you'd better take the result by value or immediately use it, otherwise you get a dangling reference. If minmax gave you a pair<int, int> here instead of a pair<int const&, int const&>, you wouldn't have any problems.
2. auto decays top-level cv-qualifiers and strips references, but it doesn't remove all the way down. Here, you're deducing that pair<int const&, int const&>, but if we had deduced pair<int, int>, we would again not have any problems.

(1) is a much easier problem to solve than (2): write your own functions to take everything by value:

template <typename T>
std::pair<T, T> minmax(T a, T b) {
    return (b < a) ? std::pair(b, a) : std::pair(a, b);
}

auto [amin, amax] = minmax(3, 6); // no problems

The nice thing about taking everything by value is that you never have to worry about hidden dangling references, because there aren't any. And the vast majority of uses of these functions are using integral types anyway, so there's no benefit to references.

And when you do need references, for when you're comparing expensive-to-copy objects... well, it's easier to take a function that takes values and force it to use references than it is to take a function that uses references and try to fix it:

auto [lo, hi] = minmax(std::ref(big1), std::ref(big2)); 

Additionally, it's very visible here at the call site that we're using references, so it would be much more obvious if we messed up.

---

While the above works for lots of types due to reference_wrapper<T>'s implicit conversion to T&, it won't work for those types that have non-member, non-friend, operator templates (like std::string). So you'd additionally need to write a specialization for reference wrappers, unfortunately.

Answer 2

What's important to note in auto [amin, amax] is that the auto, auto& and so forth are applied on the made up object e that is initialized with the return value of std::minmax, which is a pair. It's essentially this:

auto e = std::minmax(3, 6);

auto&& amin = std::get<0>(e);
auto&& amax = std::get<1>(e);

The actual types of amin and amax are references that refer to whatever std::get<0> and std::get<1> return for that pair object. And they themselves return references to objects long gone!

When you use std::tie, you are doing assignment to existing objects (passed by reference). The rvalues don't need to live longer than the assignment expressions in which they come into being.

---

As a work around, you can use something like this (not production quality) function:

template<typename T1, typename T2>
auto as_value(std::pair<T1, T2> in) {
    using U1 = std::decay_t<T1>;
    using U2 = std::decay_t<T2>;
    return std::pair<U1, U2>(in);
}

It ensures the pair holds value types. When used like this:

auto [amin, amax] = as_value(std::minmax(3, 6));

We now get a copy made, and the structured bindings refer to those copies.