-
-
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
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
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! |
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 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: |
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.
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
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