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

feat(util): Grid type-erased output and comparisons #3469

Merged

Conversation

paulgessinger
Copy link
Member

@paulgessinger paulgessinger commented Aug 1, 2024

  • Concrete Axis instances get an outstream operator
    • IAxis adds a toStream virtual method overriden by derived classes, so IAxis instances become printable without downcasting.
  • IGrid also gets a toStream virtual method + an outstream operator to allow printing a whole grid.
  • IAxis gets an equality operator
  • IGrid gets an equality operator working with the IAxis interface instead of dedicated virtual methods. This only checks the axes for equivalence, not the grid contents
  • Adds a minimal AxisConcept class, that just checks if a type inherits from IAxis.
  • IAxis gets a visit function accepting a generic callable. This function will use runtime control flow to call the callable with the concrete axis type. This allows dispatching into code with the correct concrete axis type, without having to copy the downcasting logic everywhere:
    const IAxis& someAxis;
    someAxis.visit([](auto axis) {
      // decltype(axis) is now the concrete axis type. 
    });

I think this functionality is somewhat related, which is why I grouped it like this. If there's disagreement, I'm happy to split this up further.

@paulgessinger paulgessinger added this to the v36.1.0 milestone Aug 1, 2024
@github-actions github-actions bot added the Component - Core Affects the Core module label Aug 1, 2024
andiwand
andiwand previously approved these changes Aug 1, 2024
Copy link
Contributor

@andiwand andiwand left a comment

Choose a reason for hiding this comment

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

LGTM 👍 left two minor comments

Core/include/Acts/Utilities/Grid.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Utilities/IAxis.hpp Outdated Show resolved Hide resolved
asalzburger
asalzburger previously approved these changes Aug 5, 2024
Copy link
Contributor

@asalzburger asalzburger left a comment

Choose a reason for hiding this comment

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

Looks good.

@asalzburger
Copy link
Contributor

The new code looks all tested, so I guess SonarCloud reports a bit awkwardly here?

@paulgessinger
Copy link
Member Author

@asalzburger I can add some more test for the output and equality code. Doesn't hurt anyway.

@paulgessinger paulgessinger dismissed stale reviews from asalzburger and andiwand via b5d9ffc August 6, 2024 11:27
Copy link

sonarqubecloud bot commented Aug 7, 2024

@andiwand andiwand merged commit 727d4b1 into acts-project:main Aug 7, 2024
48 checks passed
@github-actions github-actions bot removed the automerge label Aug 7, 2024
@acts-project-service acts-project-service added the Fails Athena tests This PR causes a failure in the Athena tests label Aug 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - Core Affects the Core module Fails Athena tests This PR causes a failure in the Athena tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants