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

Improve the patch for NaN with MSVC #8628

Closed
wants to merge 2 commits into from
Closed

Improve the patch for NaN with MSVC #8628

wants to merge 2 commits into from

Conversation

seisman
Copy link
Member

@seisman seisman commented Nov 21, 2024

Description of proposed changes

Not sure what's happening. Changes in PR #8627 passed in that PR, but failed after merging into the master branch (see https://github.com/GenericMappingTools/gmt/actions/runs/11934957233/job/33265224118).

ChatGPT told me:

In C, a static const variable at global scope must be initialized with a constant expression that can be evaluated at compile time. However:

(1e+300 * 1e+300) * 0.0F involves floating-point arithmetic, which is not considered a compile-time constant in most C compilers.

HUGE_VAL - HUGE_VAL is also not a compile-time constant for the same reason.

So, this PR improves the definition NAN without any floating-point arithmetic.

With the new patch, the conda-forge building in conda-forge/gmt-feedstock#302 passes.

@seisman seisman added the bug Something isn't working label Nov 21, 2024
@seisman seisman added this to the 6.6.0 milestone Nov 21, 2024
@seisman
Copy link
Member Author

seisman commented Nov 21, 2024

Hmmm, the patch still doesn't work in our CI.

@joa-quim
Copy link
Member

joa-quim commented Nov 21, 2024

The definition I found in another Windows Kits header was
#define NAN (-(float)(INFINITY * 0.0F))
where
#define INFINITY ((float)(_HUGE_ENUF * _HUGE_ENUF))
and

#ifndef _HUGE_ENUF
#define _HUGE_ENUF 1e+300 // _HUGE_ENUF*_HUGE_ENUF must overflow
#endif

My first attempt had just (no const qualifier)

#define NAN (-(float)(1e+300 * 1e+300 * 0.0F))

and that passed our CI'

Edit: I see that your change was to put it like that, but you also changed the non MSVC branch. Better not do it. If the problem is only with MSVC, restrict changes to it.

EDIT2: The other difference to my first patch is that it was applied into a different header, that presumably was included first that this one.

@seisman
Copy link
Member Author

seisman commented Nov 21, 2024

Not sure what's happening. Changes in PR #8627 passed in that PR, but failed after merging into the master branch (see https://github.com/GenericMappingTools/gmt/actions/runs/11934957233/job/33265224118).

it turns our in PR #8627, the CI was still using the old MSVC 19.41.34123.0 (that's why the CI passes), while in the master branch and this PR, the CI has upgraded to MSVC 19.42.34433.0.

@joa-quim
Copy link
Member

joa-quim commented Nov 21, 2024

OK, let's try this that is a distillation of the NAN definition in a header called corecrt_math.h

#ifndef NAN
#	ifndef INFINITY
#		ifndef _HUGE_ENUF
#			define _HUGE_ENUF 1e+300 // _HUGE_ENUF*_HUGE_ENUF must overflow
#		endif
#		define INFINITY ((float)(_HUGE_ENUF * _HUGE_ENUF))
#	endif
#	define NAN (-(float)(INFINITY * 0.0F))
#endif

@seisman
Copy link
Member Author

seisman commented Nov 21, 2024

OK, let's try this that is a distillation of the NAN definition in a header called corecrt_math.h

can we just include this header file?

@joa-quim
Copy link
Member

Sounds complicated. That header is part of Windows Kits, which I'm not even sure it belongs to MSVC (probably not).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants