62
std::tie(a, b) = std::minmax(a, b);

I think this is intuitive code. Clean and understandable. Too bad it doesn't work as intended, as std::minmax templates for const&. If therefore the values are swapped inside the std::pair<const&, const&> than one assignement will overwrite the other value:

auto[a, b] = std::make_pair(7, 5);

std::tie(a, b) = std::minmax(a, b);

std::cout << "a: " << a << ", b: " << b << '\n';

a: 5, b: 5

The expected output here is a: 5, b: 7.

I think this is important as implementing transform functions to apply a function onto some ranges requires such statements for intuitive lambdas. For example:

std::vector<int> v{ 0, 1, 0, 2, 0 };
std::vector<int> u{ 1, 0, 1, 0, 1 };

perform(v.begin(), v.end(), u.begin(), [](auto& a, auto& b){ 
    std::tie(a, b) = std::minmax(a, b);    
}); 

//v would be == {0, 0, 0, 0, 0}
//u would be == {1, 1, 1, 2, 1}

One solution I found was constructing an std::tuple explicitly without any reference qualifiers over the std::pair<const&, const&> to enforce a copy:

std::tie(a, b) = std::tuple<int, int>(std::minmax(a, b));

But this <int, int> redundancy seems rather awful, especially when having saidauto& a, auto& b before.

Is there a nice, short way to perform this assignement? Could it be that this is the wrong direction and just saying if (a >= b) { std::swap(a, b); } would be the best approach here?

Stack Danny
  • 7,754
  • 2
  • 26
  • 55
  • 8
    While the answers provided are nice, I'm somewhat unhappy about the temporaries, as these might get costly with some data types. So I personally would rather go with the `if-swap` approach. You might pack it in your own `minmax_inplace` template function (with void return type)... – Aconcagua Jun 24 '19 at 18:14
  • @Aconcagua I benchmarked [100k - 10mil elements]: without optimizations: `if-swap` was **~2.0 times faster**. with optimizations: `if-swap` was **~1.5 times faster**. So if speed is required, I would certainly go for `if-swap` – Stack Danny Jun 26 '19 at 07:33

3 Answers3

65

You can use an initializer list for minmax:

std::tie(a, b) = std::minmax({a, b});

This causes temporary objects to be created, just like when using unary plus, but has the benefit that it works with types lacking the unary plus operator too.

using namespace std::string_view_literals;

auto [a, b] = std::make_pair("foo"sv, "bar"sv);
std::tie(a, b) = std::minmax({a, b});
std::cout << "a: " << a << ", b: " << b << '\n';

Output:

a: bar, b: foo

Could it be that this is the wrong direction and just saying if (a >= b) { std::swap(a, b); } would be the best approach here?

I'd make it if(b < a) std::swap(a, b); because of the Compare1 requirement, but yes, I suspect that'll be faster and it's still very clear what you want to accomplish.

[1] Compare [...] The return value of the function call operation applied to an object of a type satisfying Compare, when contextually converted to bool, yields true if the first argument of the call appears before the second in the strict weak ordering relation induced by this type, and false otherwise.

Ted Lyngmo
  • 93,841
  • 5
  • 60
  • 108
  • 6
    This has the nice benefit that it works for more types as well (can't unary-`+` a `std::string`, for instance). – Barry Jun 24 '19 at 15:48
  • 5
    You may want to note that this causes the creation of temporary objects so it could be expensive. – NathanOliver Jun 24 '19 at 15:53
  • 2
    @NathanOliver Well, you need temporaries. Can't really get around that? Problem is that this does copies instead of moves because of `initializer_list`... Can't get around that either :-( – Barry Jun 24 '19 at 16:00
  • 1
    @Barry Well, if the OP is fine with a custom function doing a swap and returning a pair of references, or not even returning anything, would bypass any temporary creation. – NathanOliver Jun 24 '19 at 16:04
  • 2
    I agree, this is the shortest, least confusing and most maintainable approach. – Stack Danny Jun 24 '19 at 16:07
  • @TedLyngmo what's better about `b < a` than `a >= b`? Given todays optimization, does it really matter? Or is the reason readability as to indicate that the case where `a == b` is not directly adressed? – Stack Danny Jul 01 '19 at 11:48
  • 1
    @StackDanny Yes, if the type involved in the comparison has `operator>=` it'll probably be just as effective. It's just that the usual requirement on types that should be sorted is that `operator<` should exists, so for that reason alone, I prefer sticking to that operator everywhere. – Ted Lyngmo Jul 01 '19 at 11:57
29

You can enforce this with a certain level of brevity as follows.

std::tie(a, b) = std::minmax(+a, +b);

std::cout << "a: " << a << ", b: " << b << '\n';

Explanation: the builtin unary plus operator, for the sake of symmetry with its unary minus sibling, returns its operand by value (it also performs the usual arithmetic conversions, but that doesn't apply to ints). This means it has to create a temporary, even though this temporary is nothing but a copy of the operand. But for the usage of minmax in this example, it's sufficient: swapping references here doesn't assign through anymore, because the references on the right hand side (the const int& arguments passed to minmax) don't refer to the same objects as those on the left hand side (inside the tuple of references created by std::tie).

The output is as desired:

a: 5, b: 7

lubgr
  • 37,368
  • 3
  • 66
  • 117
5

Sometimes, taking a step back and finding a different way pays off:

if (b < a)
    std::iter_swap(&a, &b);

That's concise and generally more efficient, certainly at least on par. Maybe pack it into its own function:

template <class T>
void reorder(T& a, T& b)
noexcept(noexcept(b < a, void(), std::iter_swap(&a, &b))) {
    if (b < a)
        std::iter_swap(&a, &b);
}

I'm using std::iter_swap() so I don't have to use the using std::swap; swap(a, b) two-step for generality in pre-C++2a, which introduces customization point objects making that obsolete.

Deduplicator
  • 44,692
  • 7
  • 66
  • 118
  • Doesn't that just make `iter_swap` dereference the pointers back to do a normal `swap` on the objects, like `std::swap(*a, *b);`? – Ted Lyngmo Jun 26 '19 at 16:38
  • @TedLyngmo No, it uses the two-step, finding customized implementations over ADL. – Deduplicator Jun 26 '19 at 16:42
  • @TedLyngmo It's defined in `std` (as a replacement for importing `using std::swap;`), and calls `swap()` unqualified. Voila, the two-step, adapted to implementing `std`. – Deduplicator Jun 26 '19 at 17:02
  • Ok, I think I got it. For fundamental and std types, will it be able to choose a different `swap()` implementation than `swap` itself does? I was under the impression that `swap` also did some trickery to select customized implementations over the old _copy one obj to tmp ..._-thingy. – Ted Lyngmo Jun 26 '19 at 17:08
  • @TedLyngmo `std::swap()` is a lot of overloads: 1. The generic algorithm for single objects. 2. The generic algorithm for native arrays. 3. Some custom implementations for types also in `std`. The two-step, calling `std::iter_swap()` instead, or C++2a CPOs are for ensuring custom implementations for user-defined types are found by ADL. – Deduplicator Jun 26 '19 at 17:27
  • Sounds really nice. You get +1 from me for making me aware of this. Any reason to not use it over a plain `std::swap` always? Perhaps you could add some of your explanations in the answer too? It wasn't obvious what two-step meant in this context for me at least. – Ted Lyngmo Jun 26 '19 at 17:34