-
-
Notifications
You must be signed in to change notification settings - Fork 183
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
set static const values to static constexpr #2830
Conversation
…imization still treats them as known as compile time
…to fix/constexpr-constants
…to fix/constexpr-constants
@SteveBronder is this ready for review? |
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.
Few q's and comments
@@ -12,7 +12,7 @@ namespace math { | |||
namespace opencl_kernels { | |||
|
|||
// \cond | |||
static const std::string add_batch_kernel_code = STRINGIFY( | |||
static constexpr const char *add_batch_kernel_code = STRINGIFY( |
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.
static constexpr const char *add_batch_kernel_code = STRINGIFY( | |
static constexpr const char* add_batch_kernel_code = STRINGIFY( |
For consistency
@@ -10,7 +10,7 @@ namespace stan { | |||
namespace math { | |||
namespace opencl_kernels { | |||
// \cond | |||
static const std::string is_symmetric_kernel_code = STRINGIFY( | |||
static constexpr const char *is_symmetric_kernel_code = STRINGIFY( |
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.
static constexpr const char *is_symmetric_kernel_code = STRINGIFY( | |
static constexpr const char* is_symmetric_kernel_code = STRINGIFY( |
@@ -12,7 +12,7 @@ namespace math { | |||
namespace opencl_kernels { | |||
|
|||
// \cond | |||
static const char *cumulative_sum1_kernel_code = STRINGIFY( | |||
static constexpr const char *cumulative_sum1_kernel_code = STRINGIFY( |
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.
static constexpr const char *cumulative_sum1_kernel_code = STRINGIFY( | |
static constexpr const char* cumulative_sum1_kernel_code = STRINGIFY( |
@@ -62,7 +62,7 @@ static const char *cumulative_sum1_kernel_code = STRINGIFY( | |||
// \endcond | |||
|
|||
// \cond | |||
static const char *cumulative_sum2_kernel_code = STRINGIFY( | |||
static constexpr const char *cumulative_sum2_kernel_code = STRINGIFY( |
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.
static constexpr const char *cumulative_sum2_kernel_code = STRINGIFY( | |
static constexpr const char* cumulative_sum2_kernel_code = STRINGIFY( |
stan/math/prim/fun/constants.hpp
Outdated
static constexpr double LOG_PI | ||
= 1.14472988584940017414342735135305871164729481291531157151362; |
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.
This is a bit of a departure from how we've stored these constants so far, and not something I'm a huge fan of tbh, is it an essential change here?
Not a blocking comment if there's no alternative, just personal preference
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.
Actually! We can do this directly using boost's constants:
static constexpr double LOG_PI = 2 * boost::math::constants::log_root_two_pi<double>() - LOG_TWO;
stan/math/prim/fun/log.hpp
Outdated
static constexpr double inf = std::numeric_limits<double>::infinity(); | ||
static constexpr double nan = std::numeric_limits<double>::quiet_NaN(); |
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.
This header should just use the NOT_A_NUMBER
and INFTY
constants directly
stan/math/prim/fun/log10.hpp
Outdated
@@ -67,7 +67,7 @@ namespace internal { | |||
*/ | |||
template <typename V> | |||
inline std::complex<V> complex_log10(const std::complex<V>& z) { | |||
static const double inv_log_10 = 1 / std::log(10); | |||
static constexpr double inv_log_10 = 1 / 2.30258509299404568401799145468; |
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.
static constexpr double inv_log_10 = 1 / 2.30258509299404568401799145468; | |
static constexpr double inv_log_10 = 1 / LOG_TEN; |
stan/math/prim/prob/wiener_lpdf.hpp
Outdated
static constexpr double LOG_PI_LOG_WIENER_ERR | ||
= LOG_PI + -13.81551055796427410410794872810618524; |
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.
static constexpr double LOG_PI_LOG_WIENER_ERR | |
= LOG_PI + -13.81551055796427410410794872810618524; | |
static constexpr double LOG_PI_LOG_WIENER_ERR | |
= LOG_PI - 6 * LOG_TEN; |
stan/math/prim/prob/wiener_lpdf.hpp
Outdated
static constexpr double SQUARE_PI_OVER_TWO | ||
= 9.869604401089358618834490999876151135 * 0.5; |
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.
static constexpr double SQUARE_PI_OVER_TWO | |
= 9.869604401089358618834490999876151135 * 0.5; | |
static constexpr double SQUARE_PI_OVER_TWO = (pi() * pi()) / 2; |
stan/math/prim/fun/constants.hpp
Outdated
static constexpr double LOG_SQRT_PI | ||
= 0.5723649429247000870717136756765293558236474064576557857568115357; |
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.
static constexpr double LOG_SQRT_PI | |
= 0.5723649429247000870717136756765293558236474064576557857568115357; | |
static constexpr double LOG_SQRT_PI = LOG_PI / 2; |
One more simplification I thought of
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: |
@andrjohns thank you for taking this over the finish line! |
Summary
This is to fix a bug where some static initialization fiascos can happen when compiling at
-O0
. I went through and set many of the values fromstatic const
tostatic constexpr
. This should only affect things when compiling at-O0
since all the other optimizations levels treatstatic const
values as known at compile time.Tests
No new tests
Side Effects
Nope. Though I'm away from my desktop atm so these might fail the OpenCL tests as I was not able to test them locally.
Release notes
replaces
static const
objects withstatic constexpr
when available.Checklist
Math issue #(issue number)
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