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

Add real8 support to CMake #13

Merged
merged 13 commits into from
Nov 16, 2019
Merged

Add real8 support to CMake #13

merged 13 commits into from
Nov 16, 2019

Conversation

letmaik
Copy link

@letmaik letmaik commented May 8, 2019

Implements #11.

This needs some more testing.

@letmaik letmaik requested a review from dmey May 8, 2019 21:30
@dmey
Copy link

dmey commented May 9, 2019

That's fine -- I will look at this once #9 is in.

Copy link

@dmey dmey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK with me. Conditional to full checks passing after #9 is merged in.

@dmey
Copy link

dmey commented May 12, 2019

We should add at least build tests for this as well before merging this in.

@letmaik
Copy link
Author

letmaik commented May 22, 2019

I enabled build tests via Travis CI and Appveyor, where half of the configurations now build with USE_REAL8=1. This prevents adding another dimension to Azure Pipelines where we already have a lot of jobs.

Given the comments in #11, we should probably in the README change

|USE_REAL8|ON, OFF|OFF|Whether to use 8-byte reals|

to

|USE_REAL8|ON, OFF|OFF|Whether to use double precision reals|

I would still keep the name USE_REAL8 simply to be in line with what WRF does (-r8). But I'm open to suggestions. It could also be USE_DP for example.

Or we leave it as it is since we don't support platforms/compilers where real8 is not actually 8-byte reals...

@dmey
Copy link

dmey commented May 24, 2019

Or we leave it as it is since we don't support platforms/compilers where real8 is not actually 8-byte reals...

I'd leave the flag as is but I am OK with change in docs to |USE_REAL8|ON, OFF|OFF|Whether to use double precision reals|

@letmaik
Copy link
Author

letmaik commented May 24, 2019

Done. Let's target this feature for 4.1.1. So, do not merge until WRF-CMake 4.1 is released.

@dmey dmey added this to the 4.1.1 milestone May 25, 2019
@dmey
Copy link

dmey commented May 25, 2019

Agree -- added to 4.1.1 milestone to make it easier to track.

@dmey
Copy link

dmey commented Nov 14, 2019

4.1.1 and 4.1.2 have been released -- do you still want to merge this in? Do we need to create releases? We can just wait for 4.2.0 otherwise...

@letmaik
Copy link
Author

letmaik commented Nov 14, 2019

I'll merge it once CI passed. Please open a separate issue if you want to discuss updating to a new WRF version.

@letmaik letmaik merged commit ff1e19f into wrf-cmake Nov 16, 2019
@letmaik letmaik deleted the letmaik/real8 branch November 16, 2019 10:13
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.

2 participants