Skip to content

cmath: Restore definitions of std::fpclassify #42

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

Closed
wants to merge 1 commit into from

Conversation

jwakely
Copy link
Contributor

@jwakely jwakely commented May 14, 2025

This partially reverts 5a4c53d and adds the constants needed for it to work.

Fixes: #40

@jwakely jwakely mentioned this pull request May 14, 2025
This partially reverts 5a4c53d and adds
the constants needed for it to work.

Fixes: modm-io#40
@ckormanyos
Copy link
Collaborator

Hi @jwakely thanks for this.

I tried your changes locally, and they do work. I used std::fpclassify, std::signbit and also tried std::isinf, std::isnan and std::isfinite for further checks. All these are no OK.

There are some other goodies in my branch you might like to integrate into this one (or alternately) I could integrate your more proper <cmath> changes into mine.

In my branch, I also

  • modernized CI
  • and adapted the docs to reflect the corrected state of <cmath>

So you will want those changes too.

Or if you prefer, i could merge your changes into my branch manually (which I've done locally). It's your choice.

Cc: @chris-durand and @salkinium

@ckormanyos
Copy link
Collaborator

ckormanyos commented May 15, 2025

Hi @jwakely I would also like to discuss a further point with you regarding the constants such as FP_ZERO and the like.

These should come out of <math.h>. So if we leave them alone that could be OK.

There would be several other options. Let's say, however, that a client had previously worked around the missing constants earlier and did a

#define FP_ZERO 42

or something local like that. Then we would accept the definition. Do you think it makes sense to warn or error out in such cases or simply #undef them if they are #defined? Upon reflection, I think accepting a possibly wrong pre-existing definition might not be the right way to go.

Thoughts?

Cc: @chris-durand and @salkinium

@ckormanyos
Copy link
Collaborator

Hi @jwakely I fumbled around with some local, manual merges, but ended up bringing your corrected changes into #41. We can either continue there or continue with yours.

Anyway in my #41, you'll find the latest greatest <cmath>, docs and a corrected sequence that was erroring out on stl_iterator in GCC 15.1.0 locally for me.

@jwakely
Copy link
Contributor Author

jwakely commented May 15, 2025

Let's close this and continue with #41. I only created this one because it seemed clearer to show a real diff rather than just explaining what I meant.

Upon reflection, I think accepting a possibly wrong pre-existing definition might not be the right way to go.

I don't think it is really a problem to use "wrong" definitions provided by the user, as long as they are defined sensibly, as required by the C standard, i.e.

They expand to integer constant expressions with distinct values.

If the user has provided those macros and they meet that requirement, then it would be fine to use them. The definition of std::fpclassify uses __builtin_fpclassify which doesn't care what the actual values are, because you pass them into the built-in and it just returns one of those values. So any set of distinct integer constant values is fine. If the user has provided them, it would be bad to #undef them and redefine them yourself because then they'll get different values depending on #include order, i.e. before and after including <cmath>. That would be bad.

If the user has defined them so the values are not distinct, or are not integer constants, then the user gets what they deserve. I don't think that's your problem and you shouldn't need to handle that or try to make <cmath> work for such silly users.

There could potentially be a problem if the user has only defined a subset of those macros, maybe because they only cared about FP_NAN and not the others. Then they might have picked a value for FP_NAN which is equal to the value you're using for e.g. FP_ZERO. Now the values would not be distinct.

If you care about that possibility, you could use a check that none of them is defined before defining your versions:

#if !defined(FP_NAN) && !defined(FP_INFINITE) && !defined(FP_ZERO) \
  && !defined(FP_SUBNORMAL) && !defined(FP_NORMAL)
#define FP_NAN         0
#define FP_INFINITE    1
#define FP_ZERO        2
#define FP_SUBNORMAL   3
#define FP_NORMAL      4
#endif

This way you don't conflict with values provided by the user, but they'll get an error in the definition of fpclassify because some of them are missing:

  constexpr int
  fpclassify(float __x)
  { return __builtin_fpclassify(FP_NAN, FP_INFINITE, FP_NORMAL,
                               FP_SUBNORMAL, FP_ZERO, __x); }

That should prompt the user to either define all of them, or stop defining any of them, so that they come from your new #defines in <cmath>. If you wanted to be kind to users, you could detect that case and give a more user-friendly error:

#if !defined(FP_NAN) && !defined(FP_INFINITE) && !defined(FP_ZERO) \
  && !defined(FP_SUBNORMAL) && !defined(FP_NORMAL)
#define FP_NAN         0
#define FP_INFINITE    1
#define FP_ZERO        2
#define FP_SUBNORMAL   3
#define FP_NORMAL      4
#elif !defined(FP_NAN) || !defined(FP_INFINITE) || !defined(FP_ZERO) \
  || !defined(FP_SUBNORMAL) || !defined(FP_NORMAL)
#error "Some floating-point number classification macros are missing."
#error "Either define all of them or define none of them so that <cmath> can do it."
#endif

@jwakely jwakely closed this May 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

<cmath> does not publish std::fpclassify()
2 participants