Final_act has constructor taking `const Final_act&&` about gsl HOT 10 CLOSED

Yes, it should.

from gsl.

Related, but I'm pretty sure that the current implementation of finally and Final_act rely on RVO for correctness, which seems like a bad idea to me. Even if we use std::move in a proper copy move constructor as suggested here, if a temporary Final_act was created (because RVO didn't happen even though, yes, on all major compilers it would), wouldn't the destruction of that temporary Final_act cause f to be invoked immediately (and then invoked again later when the local, non-temporary Final_act goes out of scope)?

Anyway, I would think Final_act would have to look like this so that we don't rely on an optional compiler optimization for correctness:

// Final_act allows you to ensure something gets run at the end of a scope
template <class F>
class Final_act
{
public:
    explicit Final_act(F f) : f_(std::move(f)), invoke_(true) {}

    Final_act(Final_act&& other) : f_(std::move(other.f_)), invoke_(true) { other.invoke_ = false; }
    Final_act(const Final_act&) = delete;
    Final_act& operator=(const Final_act&) = delete;

    ~Final_act() { if (invoke_) f_(); }

private:
    F f_;
    bool invoke_;
};

My apologies if I have misunderstood how moving a lambda works, but I'm pretty sure I've got this right. The following code results in in the lambda being invoked twice (tested on multiple compilers):

#include <iostream>

using namespace std;

int main()
{
    auto l = [](){ cout << "Hello, lambda!" << endl; };
    auto ll = std::move(l);
    l();
    ll();
}

from gsl.

RVO isn't an optional optimization. It's actually required by the standard, and has been for a while.

edit: I'm pretty sure it got the special treatment because optimizations are generally not allowed to change behavior, whereas RVO requires that.

from gsl.

Final_act does need some improvement, but not for RVO reasons (as Treb noted).
#11 calls out one issue in its implementation and there was a related PR but it contained other changes that didn't reflect the design, so had to be rejected.

If you'd like to offer a PR that addresses #11 feel free to do so. i won't be able to take a look at fixing it for a day or so.

from gsl.

Thank you for such a prompt response!

I don't have an official copy of the standard handy, so I'm going off of the documentation on cppreference.com, which I find generally trustworthy. Here is what it says about copy elision (emphasis added):

Under the following circumstances, the compilers are permitted to omit the copy- and move-constructors of class objects even if copy/move constructor and the destructor have observable side-effects.

Later:

Copy elision is the only allowed form of optimization that can change the observable side-effects. Because some compilers do not perform copy elision in every situation where it is allowed (e.g., in debug mode), programs that rely on the side-effects of copy/move constructors and destructors are not portable.

The wording here strongly implies that copy elision is not required. Furthermore, if this copy (PDF) of the C++ standard is accurate, section 12.8.31 does not use language that indicates that copy elision is required either. Perhaps this copy is now out of date; "a while" isn't a terribly precise term. ;)

Naturally, copy elision requires special handling in the standard because it's allowed to elide copies (and moves) that have side effects. But my reading of it is that this special language permits but does not require copy elision.

Now I would dearly love to be wrong about this because it would mean that it would be easier to make the case to colleagues who are especially concerned about performance that this is an acceptable thing to do. So if I am wrong, I will gladly admit it, accept my portion of humble pie, and ask that you please refer me to the place in the C++ specification that indicates this so that I can point others to it as well. :)

As for submitting a patch, I'll see what I can do, but you may get to it before I do.

from gsl.

Ah you're right. The copy elision isn't required, but according to modern effective c++ if it doesn't elide copy, then it must treat the object as an r-value. So once the move is implemented properly we should be good.

Sorry, forgot exactly what requirements be standard had here. Not sure if I'll have time for a PR soon.

from gsl.

Yup, as long as we handle move properly, this will be golden. :) I will try to prioritize putting a PR together today.

from gsl.

I've submitted PR #90, which should resolve this issue and issue #11. It also resolves the move safety aspect of PR #13, which was rejected because the exception safety aspect was not desired.

from gsl.

Thanks for the fix!

from gsl.

Copy elision will be compulsory in C++17, but we need finally() to work now.

from gsl.