Hi there,
Does anyone know why BOOST_RESULT_OF_USE_TR1 and BOOST_NO_DECLTYPE are defined? These are required by the C++11 standard, which is the standard Stan targets, right?
I thinks those are left over from the times before C++11 or C++14. This is the commit that added them https://github.com/stan-dev/math/commit/d109ff191ddbc26ebeadecbc998156071c86e80c
Stan currently targets C++14, as far as I know we are limited by what the currently stable RTools release installs on Windows. For Rtools3.5 that is gcc 4.9.3 that is complaint with C++14.
My guess is that they can be removed, its just that no one came around removing this once the switch to C++11/C++1y/C++14 happened. We would need to remove the tests as well.
4.9.3 doesn’t support C++14. Are you sure? My understanding with 4.9.3 is that the base standard is C++11, and I have to do specific build-time checks to ensure decltype and generic lambdas are supported (https://github.com/alashworth/stan-monorepo/blob/develop/src/stan/math/CMakeLists.txt#L27).
But it’s good to know the boost defines are outdated, because I removed them and the tests that checked for them already in my branch. I assume the same is true for Google Test’s USE_OWN_TR1 define as well.
Ah yes, C++1y not C++14. Will open an issue on the Stan Math side to get this fixed there.
C++1y is just the marker for a partial implementation of C++14. We do use some C++14 features in the math library
Yeah, its just that the Stan Math readme still lists example compiling statements with c++1y.
1 Like
Actually boost::result_of is not used in Stan Math so that seems redundant regardless what compiler version or C++ standard. And BOOST_NO_DECLTYPE is redundant as of C++11.
How about
- BOOST_DISABLE_ASSERTS
- BOOST_PHOENIX_NO_VARIADIC_EXPRESSION
Are these still needed?
For BOOST_PHOENIX_NO_VARIADIC_EXPRESSION I would say yes, since I am not sure anything has changed on that front since this thread Stan-dev/stan branch develop throwing compile errors
We are now using Boost 1.69 but I dont think that makes a difference.
For BOOST_DISABLE_ASSERTS I am guessing this was added because we advise against asserts in the codebase? Cant find any discussion on that, only the commit. Just a guess tho, pinging @syclik
1 Like
Yes, this was all stuff that was added when we were C++03 compatible.
If we can get rid of outdated flags, that’s great! Things were added piecemeal. I remember the BOOST_PHOENIX_NO_VARIADIC_EXPRESSION flag (was to build stanc for Windows), but I don’t remember why we have BOOST_DISABLE_ASSERTS.
Turning off BOOST_DISABLE_ASSERTS causes tests to stop with an assert.
[----------] 13 tests from ProbDistributionsMultiStudentT
[ RUN ] ProbDistributionsMultiStudentT.NotVectorized
[ OK ] ProbDistributionsMultiStudentT.NotVectorized (1 ms)
[ RUN ] ProbDistributionsMultiStudentT.Vectorized
[ OK ] ProbDistributionsMultiStudentT.Vectorized (0 ms)
[ RUN ] ProbDistributionsMultiStudentT.Sigma
[ OK ] ProbDistributionsMultiStudentT.Sigma (0 ms)
[ RUN ] ProbDistributionsMultiStudentT.Mu
[ OK ] ProbDistributionsMultiStudentT.Mu (0 ms)
[ RUN ] ProbDistributionsMultiStudentT.Y
[ OK ] ProbDistributionsMultiStudentT.Y (0 ms)
[ RUN ] ProbDistributionsMultiStudentT.Nu
multi_student_t_test: lib/boost_1.69.0/boost/random/gamma_distribution.hpp:118: boost::random::gamma_distribution<RealType>::gamma_distribution(const result_type&, const result_type&) [with RealType = double; boost::random::gamma_distribution<RealType>::result_type = double]: Assertion `_beta > result_type(0)' failed.
[Stanubuntu16:20576] *** Process received signal ***
This happens with this test:
// nu = infinity OK
nu = std::numeric_limits<double>::infinity();
EXPECT_NO_THROW(multi_student_t_rng(nu, mu, Sigma, rng));
In this test nu = Inf which means that _beta = 2.0 / nu is 0, which asserts on BOOST_ASSERT(_beta > result_type(0));
So I think BOOST_DISABLE_ASSERT stays. We want to control when and where to throw in Stan Math rather than in less descriptive/informative asserts in Boost.
1 Like
The PR that removes all the other preprocessor defines (BOOST_RESULT_OF_USE_TR1 , BOOST_NO_DECLTYPE , GTEST_USE_OWN_TR1_TUPLE and BOOST_PHOENIX_NO_VARIADIC_EXPRESSION) passes all Stan Math and upstream tests.
Do we need to check anything else?
Do they work also on Windows / osx?
As far as I know Jenkins runs tests on Linux, Mac and Windows. Personally I have not tried it outside of Ubuntu.
1 Like
+1
There may be a different way to control this using policies. But I think that’s a major effort to update everywhere. I think this is fine for now.
I added a quick note here: https://github.com/stan-dev/math/wiki/Developer-Doc#why-do-we-have-boost_disable_asserts-included-in-the-c-build
(I think I’m gong to start updating that wiki more frequently… that should help collect some of the institutional memory.)
1 Like