-
Notifications
You must be signed in to change notification settings - Fork 199
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
Clang-tidy CI test: bump version from 16 to 17 #5600
Open
lucafedeli88
wants to merge
36
commits into
ECP-WarpX:development
Choose a base branch
from
lucafedeli88:bump_clang_tidy_ci_test_version_to_17
base: development
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Clang-tidy CI test: bump version from 16 to 17 #5600
lucafedeli88
wants to merge
36
commits into
ECP-WarpX:development
from
lucafedeli88:bump_clang_tidy_ci_test_version_to_17
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lucafedeli88
added
component: tests
Tests and CI
cleaning
Clean code, improve readability
labels
Jan 24, 2025
…idy_ci_test_version_to_16
…tidy_ci_test_version_to_17
…idy_ci_test_version_to_17
…idy_ci_test_version_to_17
Ready for rebase, v16 PR is in :) |
…idy_ci_test_version_to_17
lucafedeli88
changed the title
[WIP] Clang-tidy CI test: bump version from 16 to 17
Clang-tidy CI test: bump version from 16 to 17
Feb 3, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR bumps the version used for
clang-tidy
CI tests from 16 to 17. It also addresses all the issues found with the upgraded tool.To be merged after #5592 ✅
The issues found 🧐 and fixed 🛠️ with the upgraded tool are the following :
bugprone-switch-missing-default-case
A newly introduced check to flag
switch
statements without adefault
case (unless the argument is anenum
)cppcoreguidelines-rvalue-reference-param-not-moved
⚠️ Warning: in order to have this check compatible with performance-move-const-arg I had to set
A newly introduced check to flag when an rvalue reference argument of a function is never moved inside the function body.
performance-move-const-arg.CheckTriviallyCopyableMove
tofalse
(specifically for the three methods inablastr::utils::msg_logger
acceptingstd::vector<char>::const_iterator&& rit
arguments).misc-header-include-cycle
A newly introduced check to prevent cyclic header inclusions.
modernize-type-traits
A newly introduced check. The idea is to replace currencies of, e.g.,
std::is_integral<T>::value
, with the less verbose alternativestd::is_integral_v<T>
performance-avoid-endl
A newly introduced check. The idea is to replace
<< std::endl
with\n
, sinceendl
also forces a flush of the stream. In few cases flushing the buffer is actually the desired behavior. Typically, this happens when we want to write tostd::cerr
, which is however automatically flushed after each write operation. In cases where actually flushing tostd::cout
is the desired behavior one can do<< \n << std::flush
, which is arguably more transparent than<< std::endl
.performance-noexcept-swap
For performance reasons it is better if
swap
functions are declared asnoexcept
, in order to allow the compiler to perform more aggressive optimizations. In any case, we can use the AMReX functionamrex::Swap
, which isnoexcept
.🔄 Re-enabled checks:
This check was already available in v16, but a bug led to false positives. The bug has been corrected in v17 of the tool, so we can re-enable the check.
⛔ The PR excludes the following checks :
A newly introduced check that warns when a forwarding reference parameter is not forwarded. In order to comply with this check I think that I have to pass some parameters by reference to lambda functions inside
ParallelFor
constructs. However, this leads to issues when we compile for GPUs. Therefore, I think that the best solution is to exclude this check. See an example below (forPredFunc&& filter
):It would be awesome to include this check. However, as it is now implemented, it has no notion of "associated headers". For instance, let's suppose that the header
MyClass.H
has#include<string>
and thatMyClass.cpp
has#include "MyClass.H"
and usesstd:string
somewhere. In this case, the check raises a warning stating that you should include<string>
inMyClass.cpp
even if it is transitively included via the associate headerMyClass.H
. For this reason, for the moment, it is better to periodically check headers with theIWYU
tool.