Require C++20 and update to C++20 #698

jmcarcell · 2024-10-18T14:49:52Z

Since the Key4hep stack and the LCG stack are both on C++20 for some time, we could start using C++20 features. This PR changes the requirements and uses in some places C++20 features.

BEGINRELEASENOTES

Simplify template code by using concept and require when possible. In some places like the UserDataCollection and GenericParameters it seems that cppyy (3.1.2 with ROOT 6.32.06) doesn't like requires nor concept so they can't be changed for now.
Use consteval when possible which, unlike constexpr, guarantees evaluation at compile-time
Remove checks for versions above or below C++20 for the standard
Use algorithms from std::ranges like std::ranges::find and std::ranges::sort
Remove the ubuntu workflows since they are built on C++17

ENDRELEASENOTES

tmadlener

I am generally very in favor of this. I am not yet entirely sure how to properly reflect this in the podio version. Strictly speaking it could be a breaking change, since we remove a "feature". I think we should at least put some warning / message into a podio tag to check if someone is still building podio with c++17 (and can't easily upgrade to c++20).

We potentially have to change a few of the LCG stacks that we use to have the necessary compiler (and ROOT) support.

tmadlener

I think we potentially also have to have a discussion about some style rules around concepts, e.g. do we always use

template<typename T>
requires Concept<T>

or do we go to

template<Concept T>

where possible and only fall back to using requires when necessary?

CMakeLists.txt

include/podio/utilities/TypeHelpers.h

tmadlener · 2024-10-22T09:54:08Z

.github/workflows/ubuntu.yml

I suppose gcc11 doesn't yet have enough support for the features that we need?

It's ROOT being built with C++17 and the check failing, I'm quite sure otherwise it would pass. I think Ubuntu24 will be there soon so maybe we can check in a few days.

Ah right. In that case we could potentially add an ubuntu workflow based on a key4hep stack?

cmake/podioConfig.cmake.in

jmcarcell · 2024-10-30T20:03:29Z

I am generally very in favor of this. I am not yet entirely sure how to properly reflect this in the podio version. Strictly speaking it could be a breaking change, since we remove a "feature". I think we should at least put some warning / message into a podio tag to check if someone is still building podio with c++17 (and can't easily upgrade to c++20).

We potentially have to change a few of the LCG stacks that we use to have the necessary compiler (and ROOT) support.

I have opened another PR (#700) with a warning when building with C++17 for a future tag.

I think we potentially also have to have a discussion about some style rules around concepts, e.g. do we always use
template<typename T>
requires Concept<T>
or do we go to
template<Concept T>
where possible and only fall back to using requires when necessary?

I am against always using a Concept when possible. I think this should be something that can be reused in several places and I think for the ones created in this PR this is the case. Otherwise it becomes very hard to read if for every template you have to find out what the concept being used means. In the case when we have a reusable concept then I prefer the shorter one.

CMakeLists.txt

m-fila · 2024-11-05T08:56:51Z

I am against always using a Concept when possible. I think this should be something that can be reused in several places and I think for the ones created in this PR this is the case. Otherwise it becomes very hard to read if for every template you have to find out what the concept being used means. In the case when we have a reusable concept then I prefer the shorter one.

Core guidelines advise to always constrain template with concepts
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#t10-specify-concepts-for-all-template-arguments
and to use shorter notation for simple requirements and fallback to require when && or || of concepts are used
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#t13-prefer-the-shorthand-notation-for-simple-single-type-argument-concepts

I think the idea with using concepts everywhere is about using std concepts or building complex concepts with logic operators from them (not about introducing a separate, dedicated concept for every template argument). But I agree that introducing this policy here would be cumbersome

tmadlener · 2024-11-05T08:53:15Z

README.md


-Install ROOT 6.08.06 (or later) and set up your ROOT environment:
+Install ROOT 6.28.04 (or later) and set up your ROOT environment:


Suggested change

Install ROOT 6.28.04 (or later) and set up your ROOT environment:

Install ROOT 6.28.04 (or later) built with c++20 support and set up your ROOT environment:

tmadlener · 2024-11-05T08:56:21Z

doc/links.md

+template <typename FromU>
+requires(Mutable&& std::is_same_v<detail::GetDefaultHandleType<FromU>, FromT>&&
+             detail::isDefaultHandleType<FromU>) void setFrom(FromU value);


The formatting here should match the one in the source code

tmadlener · 2024-11-05T08:59:49Z

include/podio/GenericParameters.h

-class RNTupleReader;
-class RNTupleWriter;
-} // namespace podio
-#endif
-
-namespace podio {
-
-#if !defined(__CLING__)
-// cling doesn't really deal well (i.e. at all in this case) with the forward
-// declaration here and errors out, breaking e.g. python bindings.
-class ROOTReader;
-#endif


I am not sure I understand why these can be removed.

tmadlener · 2024-11-05T09:00:42Z

include/podio/GenericParameters.h

-#if PODIO_ENABLE_RNTUPLE
-  friend RNTupleReader;
-  friend RNTupleWriter;
-#endif
-
-#if !defined(__CLING__)
-  friend ROOTReader;
-#endif
-


Same here, these changes if they are possible seem to be unrelated to the c++17 to c++20 changes (for me). If they are independent we should move them to another PR, I think.

tmadlener · 2024-11-05T09:03:51Z

include/podio/utilities/TypeHelpers.h

  template <typename DefT, template <typename...> typename Op, typename... Args>
  using detected_or = detail::detector<DefT, void, Op, Args...>;

  template <template <typename...> typename Op, typename... Args>
-  using detected_t = typename detail::detector<nonesuch, void, Op, Args...>::type;


This is removed, because it's unused, I suppose?

tmadlener · 2024-11-05T09:06:49Z

include/podio/detail/Link.h

+  template <typename FromU>
+  requires(Mutable&& std::is_same_v<detail::GetMutableHandleType<FromT>, FromU>&&
+               detail::isMutableHandleType<FromU>) void setFrom(FromU value) {


Suggested change

template <typename FromU>

requires(Mutable&& std::is_same_v<detail::GetMutableHandleType<FromT>, FromU>&&

detail::isMutableHandleType<FromU>) void setFrom(FromU value) {

template <typename FromU>

requires(Mutable && std::is_same_v<detail::GetMutableHandleType<FromT>, FromU> &&

detail::isMutableHandleType<FromU>)

void setFrom(FromU value) {

Does this formatting get past clang-format? I think this would be much more readable than the current form. Or do we have to update our .clang-format?

tmadlener · 2024-11-05T09:08:36Z

Core guidelines advise to always constrain template with concepts https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#t10-specify-concepts-for-all-template-arguments and to use shorter notation for simple requirements and fallback to require when && or || of concepts are used https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#t13-prefer-the-shorthand-notation-for-simple-single-type-argument-concepts

I think the idea with using concepts everywhere is about using std concepts or building complex concepts with logic operators from them (not about introducing a separate, dedicated concept for every template argument). But I agree that introducing this policy here would be cumbersome

So on a quick glance we are somewhat following this already and we could just quote this as a general (albeit a bit vague) guideline?

jmcarcell added 9 commits October 18, 2024 14:15

Require C++20 in podio and ROOT

22b9c0a

Use consteval when possible and remove checks for C++20

5d37d1e

Add a few more constevals

86f4bee

Use concepts and simplify templates

0d217dc

Change enable_ifs by requires

385de71

Fix the documentation for links

525bdf7

Remove an unused header and use std::disjunction

a61d45e

Use algorithms from std::ranges

2693187

Use std::ranges in a couple more places

5eb39e2

tmadlener reviewed Oct 18, 2024

View reviewed changes

jmcarcell added 9 commits October 19, 2024 08:41

Use concepts when possible and add comments when it's not possible

d51a4bb

Simplify in more places

d65e1ae

Remove dead code

9a93f56

Use std::ranges::find

0ba74a3

Remove the ubuntu workflow since it is built with C++17

110efd5

Update docs for the frame

4c7b7b7

Fix missing endif

4415bbc

Add missing is_detected_v

a0f7f84

Fix pre-commit

2f3c035

tmadlener reviewed Oct 22, 2024

View reviewed changes

Upper-case the concept collectionType

5854520

jmcarcell mentioned this pull request Oct 30, 2024

Add a deprecation warning when building with C++17 #700

Open

jmcarcell added 2 commits October 30, 2024 21:09

Add a minimum ROOT version with support for C++20

2c7917a

Add back an ubuntu workflow with C++20

98f2b42

m-fila reviewed Oct 30, 2024

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

Update the ROOT version

68a3c81

tmadlener reviewed Nov 5, 2024

View reviewed changes

tmadlener mentioned this pull request Nov 5, 2024

Removal of c++17 support in v01-03 #701

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Require C++20 and update to C++20 #698

Require C++20 and update to C++20 #698

jmcarcell commented Oct 18, 2024 •

edited

Loading

tmadlener left a comment

tmadlener left a comment

tmadlener Oct 22, 2024

jmcarcell Oct 22, 2024

tmadlener Oct 22, 2024

jmcarcell commented Oct 30, 2024

m-fila commented Nov 5, 2024

tmadlener Nov 5, 2024

tmadlener Nov 5, 2024

tmadlener Nov 5, 2024

tmadlener Nov 5, 2024

tmadlener Nov 5, 2024

tmadlener Nov 5, 2024 •

edited

Loading

tmadlener commented Nov 5, 2024


		Install ROOT 6.08.06 (or later) and set up your ROOT environment:
		Install ROOT 6.28.04 (or later) and set up your ROOT environment:

	Install ROOT 6.28.04 (or later) and set up your ROOT environment:
	Install ROOT 6.28.04 (or later) built with c++20 support and set up your ROOT environment:

Require C++20 and update to C++20 #698

Are you sure you want to change the base?

Require C++20 and update to C++20 #698

Conversation

jmcarcell commented Oct 18, 2024 • edited Loading

tmadlener left a comment

Choose a reason for hiding this comment

tmadlener left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmcarcell commented Oct 30, 2024

m-fila commented Nov 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmadlener Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

tmadlener commented Nov 5, 2024

jmcarcell commented Oct 18, 2024 •

edited

Loading

tmadlener Nov 5, 2024 •

edited

Loading