-
Notifications
You must be signed in to change notification settings - Fork 737
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
Clean up final_act
and finally
, closes #752 and #846
#977
Conversation
We shouldn't be moving these `final_action`s around, that wasn't part of the C++CG design requirements. Went back to the simple version of `final_action`.
To satisfy some compilers. And might as well reinstate that test case.
include/gsl/util
Outdated
{ | ||
return final_action<typename std::remove_cv<typename std::remove_reference<F>::type>::type>( | ||
std::forward<F>(f)); | ||
return final_action<F>{f}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All final_action
members are noexcept(false)
, so having noexcept
here looks wrong. Yet in my opinion it would be better to have final_action
noexcept
depending on F
.
If this fixes #752 it would be good to add a test that proves this. |
@CaseyCarter would you mind taking a look at these changes and letting us know what you think? |
include/gsl/util
Outdated
{ | ||
return final_action<typename std::remove_cv<typename std::remove_reference<F>::type>::type>( | ||
std::forward<F>(f)); | ||
return final_action<F>{f}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copying f
into the constructor argument here is inconsistent with the move construction on line 55. Either we want to allow move-only functions and better support functions that are more expensive to copy than move - in which case both places should move - or we don't and both places should copy.
I'm not sure what you want me to comment on. If Guidelines C.30 is a specification, then I can point out places where this implementation doesn't agree. However, C.30 says that it is a "simplified" example so I'm not sure what the intended design is. |
Maintainers' call: Guidelines editors took a look and approved, so go ahead and address the comments and merge this. |
Applying @CaseyCarter's suggested forwarding changes And adding `[[nodiscard]]` on `finally` Thanks Casey -- somehow this slipped through the cracks for a year.
Sorry for the lag on this... @CaseyCarter Casey, I've applied your changes, thanks! @beinhaerter Werner, I've made it |
That issue is now very old and used a version of final/finally that is significantly different from the baseline for this PR; that issue no longer reproduces in the baseline either. So closed that issue. |
@CaseyCarter @dmitrykobets-msft Thanks for the feedback -- I've taken a pass at resolving the things you mentioned. How does this look now? |
Found a mistake I made in my original feedback, and then gave this a proper non-rushed review. |
Co-authored-by: Casey Carter <Casey@Carter.net>
@hsutter the changes look good - can you please rebase onto the latest main branch? There is a recent fix to the failing android test that is blocking the merge here. |
This comment was marked as resolved.
This comment was marked as resolved.
`finally` needs to use `decay_t` instead of `remove_cvref_t` so it can properly accept non-object function arguments by decaying to function pointer type. Adds test coverage for this use case which was previously missing.
Somewhere along the way, GSL's implementation of
final_act
andfinally
seems to have become way overthought. This PR is to re-simplify these facilities back to what C++ Core Guidelines C.30 said which is simple and clear and works. It just copies the invocable thing, and doesn't bother trying to optimize the copy. This should be fine, because we're typically passing something that's cheap to copy, often a stateless lambda.The problem in #846 appears to be because
finally
looks like was originally written as aconst&
/&&
overload (its state at the time that issue was opened)... to eliminate a copy when you invoke it with a temporary. If so, then the&&
was probably never intended to be a forwarder, but an rvalue reference that tripped over the horrid C++ syntax collision where a&&
parameter magically instead means a forwarding reference because the type happens to be a template parameter type here. So I suspect the original author was just trying to write an rvalue overload, and the forwarder that's there now was never intended at all.The problem in #752 just seems like a weird effect of the current complication, and with this PR that bug just disappears -- the reported example code that failed to compile now will compile and run as intended.