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

README.md

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

Updated after setting C++20 in .clang-format

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.

I didn't see the friends are needed and I was suspicious about if checking for __CLING__ was needed and it seems it's not. ROOTReader is not being used in this header.

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.

Moved to #706

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?

Yes, only used in GetCollectionType that is also removed because it's unused.

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?

Changed the format to C++20 and it looks better now

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?

Co-authored-by: Thomas Madlener <thomas.madlener@desy.de>

tmadlener reviewed Oct 18, 2024

View reviewed changes

tmadlener reviewed Oct 22, 2024

View reviewed changes

jmcarcell mentioned this pull request Oct 30, 2024

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

Merged

m-fila reviewed Oct 30, 2024

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

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

jmcarcell mentioned this pull request Nov 18, 2024

Remove unnecessary code in GenericParameters.h #706

Merged

jmcarcell force-pushed the c++20 branch from 62c1d8b to b9e6798 Compare November 18, 2024 18:07

jmcarcell added 18 commits November 18, 2024 21:23

Require C++20 in podio and ROOT

5cbfcb2

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

e4fe5b6

Add a few more constevals

360615d

Use concepts and simplify templates

f234c59

Change enable_ifs by requires

0ad6bc5

Fix the documentation for links

bd900c7

Remove an unused header and use std::disjunction

abe42b2

Use algorithms from std::ranges

91a5850

Use std::ranges in a couple more places

e92ebf6

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

b6eff0c

Simplify in more places

a5a175a

Remove dead code

95c1b24

Use std::ranges::find

7c29041

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

7c16f9e

Update docs for the frame

d9c6ec7

Fix missing endif

ed78bc0

Add missing is_detected_v

6a737eb

Fix pre-commit

e930c2e

jmcarcell and others added 7 commits November 18, 2024 21:23

Upper-case the concept collectionType

73b99e2

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

17d62d0

Add back an ubuntu workflow with C++20

468aa79

Update the ROOT version

646351c

Update README.md

e5d0991

Co-authored-by: Thomas Madlener <thomas.madlener@desy.de>

Change the format to C++20

9e2c2b8

Make sure to also format links.md

6398838

jmcarcell force-pushed the c++20 branch from b9e6798 to 6398838 Compare November 18, 2024 20:23

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

jmcarcell Nov 18, 2024

tmadlener Nov 5, 2024

jmcarcell Nov 18, 2024

tmadlener Nov 5, 2024

jmcarcell Nov 18, 2024

tmadlener Nov 5, 2024

jmcarcell Nov 18, 2024

tmadlener Nov 5, 2024 •

edited

Loading

jmcarcell Nov 18, 2024

tmadlener commented Nov 5, 2024

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

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

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