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, massive CMake cleanup #10307

Merged
merged 18 commits into from
May 23, 2020
Merged

Conversation

tamiko
Copy link
Member

@tamiko tamiko commented May 22, 2020

Closes #10296

This pull request ended up quite a bit more massive than I anticipated. Let me explain:

I think the configuration logic that I introduced with DEAL_II_WITH_CXX11, DEAL_II_WITH_CXX14, and DEAL_II_WITH_CXX17 mutated into an unmaintainable mess. The main issue is the fact that support for a language standard is controlled by the compiler flag -std=c++XY. Consequently the CMake configuration had to honor this while at the same time allowing to set the standard via DEAL_II_WITH_CXXxy. Now the problem is there are two ways of specifying the same thing and one has to start to get really smart about guessing what actually should be happen... (for the curious: We ended up implementing some very weird quinary logic thing...) To make it short I simply removed the whole damn thing. The behavior is now as follows:

  • We do not set any C++ language flag at all and thus default to whatever the compiler sets as default. This is C++14 for all compiler versions you actually want to use (Clang-6 onwards, GCC-6 onwards, https://gist.github.com/ax3l/53db9fa8a4f4c21ecc5c4100c0d93c94).
  • We now simply check whether we have C++14 support with the given configuration. If we fail detecting C++14 support we simply emit fatal error and print an error message.
  • If we detect C++17 support (for the given configuration) we export the preprocessor macro DEAL_II_HAVE_CXX17 that can be used deal.II internally for our wrapper include files, etc.

I think this is a much saner approach than the mess we had.

I will add one automagic configuration feature back, though: If we detect a very old compiler we will simply set -std=c++14 as a compiler flag unconditionally. This is safe insofar that they actually only support C++14 at maximum...

@tamiko tamiko added this to the Hackathon 2020 milestone May 22, 2020
@tamiko tamiko changed the title Require C++14 Require C++14, massive CMake cleanup May 22, 2020
Copy link
Member

@kronbichler kronbichler left a comment

Choose a reason for hiding this comment

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

This looks very good to me and has my support. I like the idea of pushing the selection of the standard to the user. Yet, it might be surprising for some users that we are now not aggressively looking for the newest standard available, so I think it might be worth documenting it on the cmake documentation web page as remarked below.

doc/news/changes/incompatibilities/20200521Maier-2 Outdated Show resolved Hide resolved
doc/users/cmake_dealii.html Show resolved Hide resolved
include/deal.II/base/config.h.in Show resolved Hide resolved
@masterleinad
Copy link
Member

Hmm...I can configure with both MSVC 2017 and 2019.

@tamiko
Copy link
Member Author

tamiko commented May 22, 2020

@masterleinad The last version seems to work - I simply setting -Werror to were we need it.

@masterleinad
Copy link
Member

Still works for me.

@tjhei
Copy link
Member

tjhei commented May 22, 2020

Let me know if I can test anything.

@drwells
Copy link
Member

drwells commented May 22, 2020

What a very interesting MSVC failure:


D:\a\dealii\dealii\include\deal.II/physics/elasticity/standard_tensors.h(72): error C2864: 'dealii::Physics::Elasticity::StandardTensors<1>::I': a static data member with an in-class initializer must have non-volatile const integral type or be specified as 'inline' [D:\a\dealii\dealii\build\source\physics\elasticity\obj_physics_elasticity_debug.vcxproj]
  D:\a\dealii\dealii\include\deal.II/physics/elasticity/standard_tensors.h(76): note: type is 'const dealii::SymmetricTensor<2,1,double>'
  D:\a\dealii\dealii\include\deal.II/physics/elasticity/kinematics.h(285): note: see reference to class template instantiation 'dealii::Physics::Elasticity::StandardTensors<1>' being compiled
  D:\a\dealii\dealii\build\source\physics\elasticity\kinematics.inst(18): note: see reference to function template instantiation 'dealii::Tensor<2,1,double> dealii::Physics::Elasticity::Kinematics::F<1,double>(const dealii::Tensor<2,1,double> &)' being compiled
D:\a\dealii\dealii\include\deal.II/physics/elasticity/standard_tensors.h(105): error C2864: 'dealii::Physics::Elasticity::StandardTensors<1>::S': a static data member with an in-class initializer must have non-volatile const integral type or be specified as 'inline' [D:\a\dealii\dealii\build\source\physics\elasticity\obj_physics_elasticity_debug.vcxproj]
  D:\a\dealii\dealii\include\deal.II/physics/elasticity/standard_tensors.h(109): note: type is 'const dealii::SymmetricTensor<4,1,double>'
D:\a\dealii\dealii\include\deal.II/physics/elasticity/standard_tensors.h(121): error C2864: 

@drwells
Copy link
Member

drwells commented May 22, 2020

There must be something off with how we now handle DEAL_II_HAVE_CXX14_CONSTEXPR.

@drwells
Copy link
Member

drwells commented May 22, 2020

FWIW the current state of this is fine on GCC 5.4.0.

@tamiko
Copy link
Member Author

tamiko commented May 22, 2020

@tamiko
Copy link
Member Author

tamiko commented May 23, 2020

This now compiles on Windows.

@tamiko
Copy link
Member Author

tamiko commented May 23, 2020

@drwells This is now rebased (no change except tracing changes to config.h.in that I made in another PR).


_cuda_ensure_feature_off(9 17 14)
_cuda_ensure_feature_off(10 17 14)
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member Author

Choose a reason for hiding this comment

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

:-)

@tamiko
Copy link
Member Author

tamiko commented May 23, 2020

4 approvals, ready to merge tag and all tests passed :-) I think I will be bold and merge my own pull request.

@tamiko tamiko merged commit b8054f3 into dealii:master May 23, 2020
@masterleinad
Copy link
Member

Let's see if hell breaks loose now. 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

switch to c++ 14
7 participants