Skip to content
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

Remove using std::cbrt to fix build on platforms which don't support it. #888

Merged
merged 1 commit into from
Dec 5, 2022

Conversation

NAThompson
Copy link
Collaborator

No description provided.

@NAThompson
Copy link
Collaborator Author

@mborland : Could you review/merge?

@@ -1011,7 +1011,6 @@ template <class T>
template<int N>
inline T constant_plastic<T>::compute(BOOST_MATH_EXPLICIT_TEMPLATE_TYPE_SPEC((std::integral_constant<int, N>)))
{
using std::cbrt;
using std::sqrt;
return (cbrt(9-sqrt(T(69))) + cbrt(9+sqrt(T(69))))/cbrt(T(18));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the target platform standard library is missing cbrt entirely will this change the behavior?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should only be in the calculate_constants.hpp file if the type T is wider than the precision of long double; so no behavior should be changed. This was the observation of @jzmaddock .

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I mean't won't the parser hit the call to cbrt and fail instead of failing at using std::cbrt? I doubt uClibc++ would be able to compile Boost.MP so I am not sure why this would be instantiated either way.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait . . . if it can't compile boost/multiprecision then shouldn't it have died much earlier in the chain?

Ultimately I think you're right: This needs to be tested by the bug reporter.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ultimately I think this is going to boil down to a standard library compliance problem. We are moving to C++14 in the next release so I don't think it's worth trying to make this work for a implementation that does not even fully support C++11.

@NAThompson NAThompson linked an issue Dec 3, 2022 that may be closed by this pull request
@jzmaddock
Copy link
Collaborator

Sorry, I mean't won't the parser hit the call to cbrt and fail instead of failing at using std::cbrt? I doubt uClibc++ would be able to compile Boost.MP so I am not sure why this would be instantiated either way.

No, the call is dependent upon the type of the template parameter, so it will do ADL when/only if instantiated, and since this code is for UDT's only, there had better be a cbrt in the same namespace as the type. In other words this change does no harm.

@mborland mborland merged commit 979d593 into develop Dec 5, 2022
@mborland mborland deleted the 885-no-cbrt branch December 5, 2022 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build failed on calculate_constants.hpp
3 participants