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

Play with C++20 concepts #14836

Merged
merged 6 commits into from Mar 3, 2023
Merged

Play with C++20 concepts #14836

merged 6 commits into from Mar 3, 2023

Conversation

bangerth
Copy link
Member

A while ago, we had a mail on the forum where someone got a linker error that turned out to be rather difficult to figure out. Ultimately, what happened is that they called a VectorTools function with many arguments from a const member function, which made the type of the solution vector a const Vector, which the compiler gladly accepted as a valid template argument -- alas, we did of course not instantiate the function with a const type because the argument was supposed to be writable.

That made me think about how we could avoid this kind of thing. The long-term solution is to use C++20 concepts that would annotate this function with a

  requires (std::is_const_v<VectorType> == false)

clause. If that clause had been there, the compiler would have provided an error message of the sort

  No such function.
  Candidate is: ...
  Candidate is not viable because requires clause is not satisfied:
            std::is_const_v<VectorType> == false
            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

That would have at least produced a compiler (rather than linker) error that is modestly helpful in finding the issue.

This patch is an attempt at playing with C++20 concepts a bit. Everything is guarded by #ifdef DEAL_II_HAVE_CXX20, so nothing should be visible to compilers that don't enable this. For the moment, I haven't gone all in at annotating things, but only trialled the following classes:

  • Triangulation: Can only be instantiated for dim>=1, spacedim<=3, dim<=spacedim
  • DoFHandler: The same
  • Point: Can only be instantiated for dim>=0.

In reality, I don't think that all that many classes need to be annotated with requires clauses because in many cases we keep passing objects around. If SolutionTransfer takes a DoFHandler as template argument, then the constraints on DoFHandler automatically carry over to SolutionTransfer: You can't instantiate SolutionTransfer<DoFHandler<3,2>> because you can't instantiate DoFHandler. Whether we do or do not annotate SolutionTransfer is then more a matter of taste or documentation.

In any case, I'm not trying to be comprehensive here, but am looking for feedback on whether that's a direction worth going. My personal opinion is that it is, in parts because we know that we want to use C++20 anyway in a few years. As an illustration of the usefulness of the general idea, I present #14828 and #14832, which are cases where we violated the constraints I had attached to Triangulation in specific instantiations.

@@ -150,6 +150,18 @@
#cmakedefine DEAL_II_HAVE_CXX17
#cmakedefine DEAL_II_HAVE_CXX20

/**
* If we have C++20 available, we can have concepts and requires
Copy link
Member

Choose a reason for hiding this comment

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

We should probably check that we define DEAL_II_HAVE_CXX20 only if the compiler supports concepts.

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean to exclude compilers that have a -std=c++20 command line flag, but do not actually support all of C++20?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, adding something using concepts in cmake/checks/check_01_cxx_features.cmake.

@drwells drwells added the C++20 label Mar 1, 2023
@@ -150,6 +150,18 @@
#cmakedefine DEAL_II_HAVE_CXX17
#cmakedefine DEAL_II_HAVE_CXX20

/**
* If we have C++20 available, we can have concepts and requires
* clauses. We want to avoid using too many #ifdef statements, so
Copy link
Member

Choose a reason for hiding this comment

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

doxygen doesn't like #ifdef here.

@bangerth
Copy link
Member Author

bangerth commented Mar 2, 2023

I added a check for concept support to cmake.

Comment on lines +85 to +95
// Test concepts and requires clauses
template <int dim, int spacedim>
concept is_valid_dim_spacedim = (dim >= 1 && spacedim <= 3 &&
dim <= spacedim);

template <int dim, int spacedim>
requires is_valid_dim_spacedim<dim,spacedim>
class Triangulation
{};

Triangulation<1,3> t;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Test concepts and requires clauses
template <int dim, int spacedim>
concept is_valid_dim_spacedim = (dim >= 1 && spacedim <= 3 &&
dim <= spacedim);
template <int dim, int spacedim>
requires is_valid_dim_spacedim<dim,spacedim>
class Triangulation
{};
Triangulation<1,3> t;
#if !(defined __cpp_concepts) || (__cpp_concepts < 201907L)
# error \"insufficient support for C++20\"
#endif

for conformity (might also be slightly faster).

Copy link
Member Author

Choose a reason for hiding this comment

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

No, I actually would like to keep it as is. If we don't trust that -std=c++20 works, then we also shouldn't trust that __cpp_concepts is set correctly. Let's test for the actual feature we want to use.

Copy link
Member

Choose a reason for hiding this comment

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

The point of these CMake features check really is that we decide if C++20 is supported based on the capabilities of the compilers with the given flags and not because the user requested C++20 support in any other way (at the moment). Thus, I wouldn't say that we don't trust that -std=c++20 works (since compilers allow passing this flag without supporting all of C++20 anyway). On the other hand, I think that the __cpp_concepts feature check is reliable, though. Anyway, I rather wanted to suggest using that than requesting changes.

@blaisb
Copy link
Member

blaisb commented Mar 3, 2023

Such a good idea!

@kronbichler
Copy link
Member

I like this direction.

@tamiko tamiko merged commit 85e854a into dealii:master Mar 3, 2023
@tamiko
Copy link
Member

tamiko commented Mar 3, 2023

I just merged it 😄

@bangerth bangerth deleted the concepts branch March 3, 2023 18:55
@bangerth
Copy link
Member Author

bangerth commented Mar 3, 2023

Well, I had assumed that this would generate more discussion :-)

I'll plod along and add some annotations to functions on occasion then!

@bangerth
Copy link
Member Author

bangerth commented Mar 3, 2023

Part of #14840.

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.

None yet

6 participants