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

Using C++20 in QC #2018

Merged
merged 7 commits into from
Jun 25, 2024
Merged

Using C++20 in QC #2018

merged 7 commits into from
Jun 25, 2024

Conversation

knopers8
Copy link
Collaborator

Now mostly as a demonstrator, since we need O2 & ROOT to move first.

I can confirm though that the QC library compiles fine and passes the tests in o2-qc-test-core

@knopers8 knopers8 requested a review from Barthelemy as a code owner October 18, 2023 07:08
knopers8 added 6 commits June 24, 2024 10:14
It did not really need c++20, but had a related todo comment.
Initially, I assumed that using the concrete type as an argument would prevent us from using it as const, ref, const ref, but actually c++ converts it properly.
Applied only to simple cases, but we could probably rewrite more with additional time investment.
@knopers8 knopers8 requested a review from justonedev1 as a code owner June 24, 2024 08:45
@knopers8 knopers8 changed the title [WIP] Using C++20 in QC Using C++20 in QC Jun 24, 2024
@knopers8
Copy link
Collaborator Author

Rebasing an old PR with some attempts at C++20...

}

/// Produces the most constrained Activity which will match all the provided
template <typename Iter, typename Accessor = const Activity& (*)(const Activity&)>
Activity strictestMatchingActivity(
Iter begin, Iter end, Accessor accessor = [](const Activity& a) -> const Activity& { return a; })
Copy link
Collaborator

Choose a reason for hiding this comment

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

why was accessor removed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The accessor was added in the first place because we did not have a way to easily pass some kind of iterator of all the activities if they are members bigger structures. std::ranges allow for that, as it is demostrated in the test, so we can remove this and expect a RangeOfActivities instead.

template <class... Args, class Enable = std::enable_if_t<(... && std::is_convertible_v<Args, DataSourceType>)>>
bool isOneOf(Args... dataSourceType) const

template <class... DataSourceType>
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't there be any concept similarly to computeNumericalActivityField ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why? this method is a member of DataSourceSpec and expects DataSourceType only, but it can accept one or more of those.

Do you mean just saying something like requires T == DataSourceType)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the "todo" comment was probably my lack of awareness, that it could be done simpler.

Copy link
Collaborator

Choose a reason for hiding this comment

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

well, there was a structure that checks whether the template args can be converted to the enum, so I thought that you want to keep the functionality

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no, not really. i think i had thought that one needs is_convertible to make it work also with const DataSourceType or const DataSourceType&, which apparently is not needed in the end.

@Barthelemy Barthelemy merged commit eebcd92 into AliceO2Group:master Jun 25, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants