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

__STDC_VERSION__ check in C++ code #9213

Closed
0-wiz-0 opened this issue Sep 3, 2018 · 2 comments · Fixed by #16551
Closed

__STDC_VERSION__ check in C++ code #9213

0-wiz-0 opened this issue Sep 3, 2018 · 2 comments · Fixed by #16551
Labels
Build issues Issues with building from source, including different choices of architecture, compilers and OS
Milestone

Comments

@0-wiz-0
Copy link

0-wiz-0 commented Sep 3, 2018

Havard Eidnes added a patch to the pkgsrc package for scipy with the following comment:

Add a patch which fixes an obviously bogus preprocessor conditional;
in our case, __STDC_VERSION__ isn't defined when built as C++.
The fix isn't completely right, it insists on <fenv.h> if built as C++.
Not entirely unreasonable, and makes this build on NetBSD/powerpc as well,
which doesn't like the redefinition of fegetround() and fesetround().

I don't know the proper fix.

Here's his patch:

--- scipy/special/_round.h.orig 2018-05-05 17:10:11.000000000 +0000
+++ scipy/special/_round.h
@@ -49,7 +53,7 @@ double add_round_down(double a, double b
 
 
 /* Helper code for testing _round.h. */
-#if __STDC_VERSION__ >= 199901L
+#if (defined(__STDC_VERSION__) && __STDC_VERSION__ >= 199901L) || defined(__cplusplus)
 /* We have C99 */
 #include <fenv.h>
 #else
@pvanmulbregt pvanmulbregt added the Build issues Issues with building from source, including different choices of architecture, compilers and OS label Sep 3, 2018
@sturlamolden
Copy link
Contributor

sturlamolden commented Nov 29, 2018

The C++ standard say that definition of __STDC_VERSION__ is implementation dependent. I see that we have cases of C include files that use this macro and are imported in C++, e.g. scipy/special/_round.h and scipy/_lib/_c99compat.h, and probably others.

First, it is not en error to use __STDC_VERSION__ in C++ because the standard allows this macro to be defined. We just have to accept that it will not build on some arcane systems.

Second, we can of course check for __cplusplus and define __STDC_VERSION__ if needed. However, the question is then to decide which value is appropriate, which would also depend on the arcane C++ implementation in question. We would then need preprocessor pragmas for any thinkable implementation, i.e. worst common denominator programming. SciPy is not going to be like OpenSSL if I have a say. Let ill behaving systems break on build-time. That is far better than introducing potential run-time errors that will materialize according to Murphy's law. Say we define __STDC_VERSION__ but accidentally give it the wrong value for some arcane system we don't know about. Then we can just sit back and wait for the next Ariane 5 incident to happen.

rgommers added a commit to rgommers/scipy that referenced this issue Jul 5, 2022
This check should always be conditional (as it is everywhere else
in the code base and in the Boost submodule), because it does not
have to be defined according to the C standard.

Closes scipygh-9213
@rgommers
Copy link
Member

rgommers commented Jul 5, 2022

Somehow I managed to miss or ignore this issue for the last 4 years, apologies. The patch looks fine to me, and there's no problem with including a simple one line change. It's indeed a bug to unconditionally use __STDC_VERSION__.

Included in gh-16551.

@rgommers rgommers added this to the 1.9.0 milestone Jul 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build issues Issues with building from source, including different choices of architecture, compilers and OS
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants