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

gmtlogo: Create the gmt_letters array dynamically to avoid MSVC compiler error #8629

Merged
merged 3 commits into from
Nov 22, 2024

Conversation

seisman
Copy link
Member

@seisman seisman commented Nov 22, 2024

As reported in conda-forge/gmt-feedstock#302 (comment), building GMT source code with MSVC started to fail with following errors:

%SRC_DIR%\src\gmtlogo.c(38): error C2099: initializer is not a constant
%SRC_DIR%\src\gmtlogo.c(131): error C2099: initializer is not a constant
%SRC_DIR%\src\gmtlogo.c(145): error C2099: initializer is not a constant

I believe it's caused by behavior changes of new MSVC versions. In PR #8627 and #8628, we tried to improve the handling of NAN on Windows to avoid the error, but it seems we don't have a good solution there.

Instead, we can refactor the gmt_letters array in gmtlogo.c to avoid initializing gmt_letters with NAN. This is done by splitting the gmt_letters array into three separate arrays (for the letter G, M and T, respectively), and then creating the gmt_letters array by joining the three arrays with NAN records. This PR also reverts the changes in PR #8627

Now the build passes.

@seisman
Copy link
Member Author

seisman commented Nov 22, 2024

The failures on macOS is unrelated and can be ignored.

@seisman seisman added the bug Something isn't working label Nov 22, 2024
@seisman seisman added this to the 6.6.0 milestone Nov 22, 2024
@joa-quim
Copy link
Member

We now have some warnings that I don't fully understand. If it works a simpler solution would have been to do

const double d_nan = NAN;

and use d_nan instead of NAN

@seisman
Copy link
Member Author

seisman commented Nov 22, 2024

I don't see any related warnings in the CI jobs, on both Linux and Windows. What are exactly the warnings?

@joa-quim
Copy link
Member

Hmm, I don't see them anymore. It was something about possible overflows.
I also see these for sometime (but they are new) that I don't understand.

D:\a\gmt\gmt\src\gmt_init.c(3754): warning C4129: 'g': unrecognized character escape sequence
D:\a\gmt\gmt\src\gmt_init.c(3755): warning C4129: 'g': unrecognized character escape sequence

@seisman
Copy link
Member Author

seisman commented Nov 22, 2024

Hmm, I don't see them anymore. It was something about possible overflows.

Yes, it's caused by typos and has been fixed in cd21d88.

I also see these for sometime (but they are new) that I don't understand.

D:\a\gmt\gmt\src\gmt_init.c(3754): warning C4129: 'g': unrecognized character escape sequence
D:\a\gmt\gmt\src\gmt_init.c(3755): warning C4129: 'g': unrecognized character escape sequence

I also see these warnings, but they're unrelated to changes in this PR.

@seisman seisman merged commit de1b538 into master Nov 22, 2024
14 of 18 checks passed
@seisman seisman deleted the fix/gmtlogo branch November 22, 2024 12:29
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