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

Require C++14 #1830

Merged
merged 5 commits into from
Nov 1, 2019
Merged

Require C++14 #1830

merged 5 commits into from
Nov 1, 2019

Conversation

ZedThree
Copy link
Member

C++14 is now widely supported by the majority of platforms and compilers:
https://en.cppreference.com/w/cpp/compiler_support#cpp14
Notably, IBM XL C++ only has partial support.

It's very much a "bugfix release" of the standard. Here's a good summary of the new features:
https://github.com/AnthonyCalandra/modern-cpp-features/blob/master/CPP14.md

As of the end of 2019, it seems not enough supercomputers have compilers that support C++17 to justify us moving to that.


This PR is a little bit longer than I was expecting, and there's a couple of changes that might be a little surprising at first.

For CMake, this was literally just changing a single character.

For autoconf... I decided to also update the macro from autoconf-archive that we use. It turns out we were using a slightly modified form (probably my doing) that added a variable CXX11_FLAGS that stored the compiler flag (-std=c++11 or similar).
The actual macro just adds the flag directly to CXX. But we use MPICXX as the compiler.

Well, it turns out we actually only need MPICXX to find the MPI compiler, and after that CXX is set to MPICXX. This is all internal to configure and the user doesn't need to know any of this, so it's safe to change our uses of MPICXX to CXX. Using ./configure MPICXX=/path/to/mpi/compiler is still supported.

Note this removes CXX11_FLAGS that I think we added
MPICXX sets the MPI compiler, but this is used to set CXX. We should
use CXX because some of the macros add flags directly to CXX
@ZedThree ZedThree added this to the BOUT-5.0 milestone Oct 31, 2019
@ZedThree
Copy link
Member Author

Compiler ICE :(

Also looks like mpic++ isn't using g++-7 in that job, so that's doubly annoying.

@bendudson
Copy link
Contributor

index_derivs strikes again :)

index_derivs.cxx:71:1:   required from here
../../include/bout/template_combinations.hxx:156:28: internal compiler error: in tsubst_copy, at cp/pt.c:13039

Make members of DerivativeType<FF> and FF `static constexpr`
instead. And because we're not using C++17 yet, we also need to add
redundant definitions outside the classes too

Also need to move the declarations of func in DerivativeType<FF> above
the uses because something else goes wrong. Not getting inlined?
Linker getting confused? Don't know, but this fixes it.

Alternative to last para: make `apply`:

    return FF{}(...);

Probably free/cheap/inline-able? Don't need func member then, but
could be useful, plausibly
@ZedThree
Copy link
Member Author

ZedThree commented Nov 1, 2019

Because nothing's ever easy, I've had to modify index_derivs a little in order to work around a bug in gcc5 that's only present in C++14 mode. It's fixed in other versions, but won't be backported.

Make members of DerivativeType and FF static constexpr
instead. And because we're not using C++17 yet, we also need to add
redundant definitions outside the classes too

Also need to move the declarations of func in DerivativeType above
the uses because something else goes wrong. Not getting inlined?
Linker getting confused? Don't know, but this fixes it.

Alternative to last para: make apply:

return FF{}(...);

Probably free/cheap/inline-able? Don't need func member then, but
could be useful, plausibly

A nice consequence of this is we can also eliminate the branching in registerMethod for nGuards, because it's now constexpr. Yey! Will do that in another PR, because it's technically unrelated.

@ZedThree ZedThree merged commit 576f9dd into next Nov 1, 2019
@ZedThree ZedThree deleted the use-c++14 branch November 1, 2019 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants