-
Notifications
You must be signed in to change notification settings - Fork 733
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Final_act has constructor taking const Final_act&&
#42
Comments
Yes, it should. |
Related, but I'm pretty sure that the current implementation of 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();
} |
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. |
Final_act does need some improvement, but not for RVO reasons (as Treb noted). 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. |
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):
Later:
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. |
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. |
Yup, as long as we handle move properly, this will be golden. :) I will try to prioritize putting a PR together today. |
Thanks for the fix! |
Copy elision will be compulsory in C++17, but we need finally() to work now. |
Shouldn't it be
Final_act(Final_act&& other)
so that it's a proper move constructor? Also shouldn't we std::move from other.f_ into f_?The text was updated successfully, but these errors were encountered: