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

Update/improve ccmath configuration. #1045

Merged
merged 3 commits into from
Nov 5, 2023
Merged

Update/improve ccmath configuration. #1045

merged 3 commits into from
Nov 5, 2023

Conversation

jzmaddock
Copy link
Collaborator

No description provided.

@jzmaddock
Copy link
Collaborator Author

@mborland most of the changes here are to ccmath: does this look OK to you? The drone failures are due to github falling over yesterday BTW, I don't see an easy way to re-run that?

@mborland
Copy link
Member

mborland commented Nov 4, 2023

@mborland most of the changes here are to ccmath: does this look OK to you? The drone failures are due to github falling over yesterday BTW, I don't see an easy way to re-run that?

It looks good to me. I have organizational access to drone so I restarted the build. The status should be updated here on re-run.

@mborland
Copy link
Member

mborland commented Nov 4, 2023

By the way once you merge the PR to merge to master for next release (#1042) will also automatically pull your commits in.

@jzmaddock
Copy link
Collaborator Author

Thanks Matt, one last failure is a network timeout, merging...

@jzmaddock jzmaddock merged commit 11e2348 into develop Nov 5, 2023
58 checks passed
@ckormanyos
Copy link
Member

I don't know why this was never a problem before, but nested namespaces are C++17 and I just had a MSVC compiler report this when set to C++14 at lines like this

@ckormanyos
Copy link
Member

Ah OK. It should be caught by the preprocessor. Now how on earth did I set a MSVC compiler to C++14 and still reach that line of code? I will investigate...

@ckormanyos
Copy link
Member

ckormanyos commented Nov 5, 2023

OK. John and Matt. Sorry to report, ... but There seems to be some kind of very obscure problem. Sadly I must report it.
@jzmaddock and @mborland

  • I take VS2019 and make a test project.
  • Set language standard to C++14
  • Then #include <boost/math/ccmath/isinf.hpp>
  • But I include isinf.hpp only after including number from multiprecision.

And I get this:

grafik

@ckormanyos
Copy link
Member

Oh well, you know, MSVC doesn't really adhere to the well-known __cplusplus values. So maybe something there?

Anyway, here is the complete code:

#include <boost/multiprecision/number.hpp>
#include <boost/math/ccmath/isinf.hpp>

auto main() -> int
{
}

@ckormanyos
Copy link
Member

Sorry guys this is really garbled since it literally just crossed my path and I need to isolate better.

This!!! is the offending code:

#define BOOST_MP_STANDALONE
#define BOOST_MATH_STANDALONE

#include <boost/config.hpp>
#include <boost/multiprecision/number.hpp>
#include <boost/math/ccmath/isinf.hpp>

auto main() -> int
{
}

grafik

@jzmaddock
Copy link
Collaborator Author

It's the standalone bit that does this - it means we have no Boost.Config, so either we need to completely copy all of it's detection logic, or accept that the helpful #error message doesn't get triggered in this case, and rely on the compiler rejecting C++17 ism's which it does perfectly well IMO.

I might be able to improve things a little for latest MSVC releases which have the C++20 feature detection macros, but not for older releases which do not. Not sure it's completely worth it anyway, the existing error message is pretty clear that this needs C++17 to compile?

@jzmaddock
Copy link
Collaborator Author

Ah... except that breaks client code that relies on BOOST_MATH_NO_CCMATH being set when the header isn't supported.... there may be an easy fix too, give me a moment!

jzmaddock added a commit that referenced this pull request Nov 5, 2023
@jzmaddock
Copy link
Collaborator Author

Improvement in #1046.

@ckormanyos
Copy link
Member

Ah... except that breaks client code that relies on BOOST_MATH_NO_CCMATH being set when the header isn't supported.... there may be an easy fix too, give me a moment!

Thank you John. I can confirm for my client code, it is fixed in your new PR #1046

@NAThompson NAThompson deleted the ccmath_config branch January 24, 2024 21:33
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.

None yet

3 participants