-
-
Notifications
You must be signed in to change notification settings - Fork 195
Use Perfect Forwarding in all functions that use apply
family of functors
#3221
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
base: develop
Are you sure you want to change the base?
Conversation
@SteveBronder anything I can do to help this over the line? |
Right now it is just making sure the tests pass. I think I got it so we should be good! |
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.
Gonna leave the proper review to @andrjohns, just two things that stood out
@@ -17,6 +17,7 @@ namespace math { | |||
template <typename ValueType> | |||
class complex_base { | |||
public: | |||
auto __rep() const { return *this; } |
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.
I can't figure out from searching what this would do
Math pipeline looks good, upstream has some (I believe legitimate) failures. All seem to blame
|
Yes I missed one |
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
It looks like |
Yes it looks like there was a conflict with some code using |
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
Summary
This fixes #3208 by using perfect forwarding for all functions that use our underlying apply family of functions for calling functions on containers and containers and containers. The issue was that the
Holder
class, when used in the apply functors, did not have enough type information to know which arguments it should take ownership of.Consider the following function, where all types are passed in via constant reference.
Calling this function with an Eigen expression that has a temporary in it would not give
apply_scalar_binary
and theHolder
inside ofapply_scalar_binary
enough information to know that theHolder
class should own any of the input arguments. As an example we can look at a simplified version of the code used inpoisson_lccdf.hpp
.gamma_p
usesapply_scalar_binary
andlog
usesapply_scalar_unary
. We need to make sure the inputs and results of thegamma_p
function do not fall out of scope by the time we go throughlog
and then assign tolog_Pi
. Before this PR it would be possible for the expressionn_val + 1.0
to fall out of scope as well as the result ofgamma_p
to go out of scope fromlog
afterlog_Pi
is assigned.To combat this we now use perfect forwarding for all of the functions that use our internal
apply
family of functors. This should allow theHolder
used internally by the apply functors to know which types need to be owned by it to make sure things do not fall out of scope.Tests
There is no new tests for this. Since it is an isue on gcc I do wonder how we should test this in our CI/CD?
Side Effects
I'd like to think of some test we can write so that, in the future, developers do not accidentally write functions that use the apply family of functors that do not use perfect forwarding.
Release notes
Adds perfect forwarding to all functions that use the apply family of functors.
Checklist
Copyright holder: Steve Bronder
The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:
- Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
- Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)
the basic tests are passing
./runTests.py test/unit
)make test-headers
)make test-math-dependencies
)make doxygen
)make cpplint
)the code is written in idiomatic C++ and changes are documented in the doxygen
the new changes are tested