Use Perfect Forwarding in all functions that use apply family of functors#3221
Use Perfect Forwarding in all functions that use apply family of functors#3221SteveBronder merged 56 commits intodevelopfrom
apply family of functors#3221Conversation
|
@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! |
WardBrian
left a comment
There was a problem hiding this comment.
Gonna leave the proper review to @andrjohns, just two things that stood out
|
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: |
|
I'm running the distribution tests on this branch with all the tests enabled + |
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: |
|
Looks like that full run passed 👍🏻 @andrjohns are you able to review again? Thanks! |
WardBrian
left a comment
There was a problem hiding this comment.
I went through all of @andrjohns comments to confirm they were addressed, and a few other small things stood out to me
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: |
WardBrian
left a comment
There was a problem hiding this comment.
Just gave this a once over and it looks good to me now. We can wait a couple days before merging if you want to give @andrjohns another chance to look
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: |
|
@SteveBronder it looks like a few of the SoA signatures aren't compiling any more: All of the errors are |
|
I'm guessing it is because the rev specializations of those functions hardcode |
|
Jenkins also failed during the merge in this repo: |
|
@SteveBronder do you have a sense of how easy this would be to fix? If it ends up being complicated I would advocate for reverting this and leaving math as-is in the release, for the sake of getting all the other bugfixes in the last 8 months in the hands of users |
I'm looking at these functions and idt The Bessel function one I think is a real error from ADL since the perfect forwarding function is being chosen over the others. I can open up a PR for that one rn |
Are you sure? They've been marked as SoA supported since the first compiler pr stan-dev/stanc3#955. |
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
Holderclass, 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_binaryand theHolderinside ofapply_scalar_binaryenough information to know that theHolderclass 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_pusesapply_scalar_binaryandlogusesapply_scalar_unary. We need to make sure the inputs and results of thegamma_pfunction do not fall out of scope by the time we go throughlogand then assign tolog_Pi. Before this PR it would be possible for the expressionn_val + 1.0to fall out of scope as well as the result ofgamma_pto go out of scope fromlogafterlog_Piis assigned.To combat this we now use perfect forwarding for all of the functions that use our internal
applyfamily of functors. This should allow theHolderused 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