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

chore: clang-tidy performance-move-const-arg #1359

Merged

Conversation

paulgessinger
Copy link
Member

@paulgessinger paulgessinger commented Aug 2, 2022

This enables the performance-move-const-arg check in clang-tidy.
It suggests replacing/removing std::move where it won't actually do anything, e.g. because the called function takes a const reference anyway, or the underlying type is trivially copyable. This PR removes the std::move calls in these cases.

@paulgessinger paulgessinger added this to the next milestone Aug 2, 2022
@paulgessinger paulgessinger changed the title clang-tidy: performance-move-const-arg chore: clang-tidy performance-move-const-arg Aug 2, 2022
@codecov
Copy link

codecov bot commented Aug 2, 2022

Codecov Report

Merging #1359 (2bbc2b3) into main (0c76d37) will decrease coverage by 0.00%.
The diff coverage is 37.50%.

@@            Coverage Diff             @@
##             main    #1359      +/-   ##
==========================================
- Coverage   47.49%   47.49%   -0.01%     
==========================================
  Files         375      375              
  Lines       19826    19824       -2     
  Branches     9282     9282              
==========================================
- Hits         9417     9415       -2     
  Misses       4035     4035              
  Partials     6374     6374              
Impacted Files Coverage Δ
Core/include/Acts/Surfaces/ConeSurface.hpp 100.00% <ø> (ø)
Core/src/Material/MaterialGridHelper.cpp 68.57% <0.00%> (-0.23%) ⬇️
Core/src/Material/VolumeMaterialMapper.cpp 7.87% <0.00%> (ø)
Core/src/Surfaces/DiscSurface.cpp 30.96% <ø> (-0.35%) ⬇️
Core/src/Surfaces/ConeSurface.cpp 34.28% <50.00%> (ø)
Core/src/MagneticField/SolenoidBField.cpp 69.09% <100.00%> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

Copy link
Contributor

@AJPfleger AJPfleger 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 to me in general.

Maybe you could add in the description, that you also changed all relevant move()-cases in this PR.

As far as I can remember, the CylinderSurface.* are very similar to the ConeSurface.*, and the cbounds are there static too. But it seems you haven't touched them in this PR.

@paulgessinger
Copy link
Member Author

As far as I can remember, the CylinderSurface.* are very similar to the ConeSurface.*, and the cbounds are there static too. But it seems you haven't touched them in this PR.

In CylinderSurface I think the shared pointer is either moved from a copy (not a const ref) or the const ref is not moved, so the clang-tidy warning doesn't trigger there.

@AJPfleger AJPfleger added Improvement Changes to an existing feature Infrastructure Changes to build tools, continous integration, ... automerge labels Aug 2, 2022
@kodiakhq kodiakhq bot merged commit c6d189d into acts-project:main Aug 2, 2022
@paulgessinger paulgessinger modified the milestones: next, v19.6.0 Aug 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Improvement Changes to an existing feature Infrastructure Changes to build tools, continous integration, ...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants