Add clang tidy CI test & tools to use it locally by lucafedeli88 · Pull Request #1303 · Hi-PACE/hipace

lucafedeli88 · 2025-10-27T14:01:23Z

This PR adds a .clang-tidy configuration file and a script to run clang-tidy locally, as well as a CI test that runs clang-tidy with the rules specified in the configuration file. Some issues found with the tool are also fixed.

The clang-tidy configuration file at the moment enables most of the checks of the bugprone- family, as well as cppcoreguidelines-avoid-goto . More checks will be added in future PRs.

Checks: '
    -*,
    bugprone-*,
        -bugprone-easily-swappable-parameters,
        -bugprone-implicit-widening-of-multiplication-result,
        -bugprone-misplaced-widening-cast,
        -bugprone-narrowing-conversions,
    cppcoreguidelines-avoid-goto
    '

HeaderFilterRegex: 'src[a-z_A-Z0-9\/]+\.H$'

The script tools/runClangTidy.sh writes a small clang-tidy wrapper in order to avoid using it on AMReX and OpenPMD source files. This script can be run by doing:

./tools/runClangTidy.sh

The compilation is done in the folder build_clang_tidy and the output of the tool is written to build_clang_tidy/clang-tidy.log

The new clang-tidy CI test is define in github/workflows/clangtidy.yml (note that since we're compiling with clang in this test I have removed the comments at the end of github/workflows/linux.yml and I have updated the setup/ubuntu_clang.sh script to install the new required dependencies).

The PR also fixes these issues found with the new CI test:

src/mg_solver/HpMultiGrid.H : a default case is added to two switch statements to respect the rule bugprone-switch-missing-default-case
src/mg_solver/HpMultiGrid.H and src/mg_solver/HpMultiGrid.cpp two variables are declared only if AMREX_USE_GPU is defined (otherwise they are flagged as unused by clang)
src/utils/MultiBuffer.cpp : ~~checks are added to respect the rule bugprone-unchecked-optional-access~~ a NOLINT is used to silence an inappropriate [bugprone-unchecked-optional-access] warning.
~~src/particles/beam/BeamParticleContainer.cpp and src/particles/plasma/PlasmaParticleContainer.cpp : remove two unused variables~~ already done in Remove unused PhysConst #1317

This PR is a follow-up of #1302

Small enough (< few 100s of lines), otherwise it should probably be split into smaller PRs
Tested (describe the tests in the PR description)
Runs on GPU (basic: the code compiles and run well with the new module)
Contains an automated test (checksum and/or comparison with theory)
Documented: all elements (classes and their members, functions, namespaces, etc.) are documented
Constified (All that can be const is const)
Code is clean (no unwanted comments, )
Style and code conventions are respected at the bottom of https://github.com/Hi-PACE/hipace
Proper label and GitHub project, if applicable

…se_clang_tidy

tools/runClangTidy.sh

…_use_clang_tidy

…se_clang_tidy

lucafedeli88 · 2025-11-20T14:39:33Z

Hei @AlexanderSinn, do you think that this could be a useful tool?

AlexanderSinn · 2025-11-20T15:03:21Z

The way I currently think about it, it is only useful if it runs as part of CI and has some more powerful checks enabled that can detect and prevent actual potential bugs. If it is just a script, then I think it will rarely be used in practice and constantly break over time from PRs not using the script.

However, as a CI test, it should finish in less than 15 minutes (for the case of many checks enabled but no errors found) to not be slower compared to the other CI tests that we have.

lucafedeli88 · 2025-11-21T11:56:56Z

The way I currently think about it, it is only useful if it runs as part of CI and has some more powerful checks enabled that can detect and prevent actual potential bugs. If it is just a script, then I think it will rarely be used in practice and constantly break over time from PRs not using the script.

However, as a CI test, it should finish in less than 15 minutes (for the case of many checks enabled but no errors found) to not be slower compared to the other CI tests that we have.

Oh, yes, sorry, I failed to explain it properly.

My intention is to finally (either in this PR or in another one) make a CI test that runs this script. The advantage of also having the script in tools is that you can run it locally with just one command (e.g., I used it for these PRs: #1313 , #1308 ).

We can progressively add more checks to .clang-tidy . hipace is already quite "clean", so we should be able to enable a lot of checks with minimal to no changes.

…se_clang_tidy

lucafedeli88 · 2025-11-26T15:01:05Z

@AlexanderSinn , what do you think about this ? I've added a new CI test that uses clang-tidy

…se_clang_tidy

lucafedeli88 · 2025-12-05T15:48:19Z

Ping, @AlexanderSinn ;-)

.github/workflows/clangtidy.yml

AlexanderSinn · 2025-12-08T14:32:56Z

src/utils/MultiBuffer.cpp

                             num_particles * sizeof(std::uint64_t));
+            }
+            else{
+                amrex::Abort("bo.m_beam_idcpu[" + std::to_string(b) + "] has no value!");


This seems much worse than the original code. The .value() function was used deliberately to get the value if it exists and give an error if it doesn't. The way the code is now, an error should never be triggered; it is there only to catch mistakes if changes are made to it. The same is done below with std::map by using .at() instead of operator[]. The .has_value() and amrex::Abort combination does the same thing in a much more verbose way. Can you use the original version with .value() and add a NOLINT comment, here and below? It would be good to give an example of how to do that.

I see the intent, now. Then the best solution is probably to add the option IgnoreValueCalls for this check. What do you think ?

(instead of a NOLINT)

Which check is this from?

bugprone-unchecked-optional-access

btw, do you prefer writing explicitly all the enabled & disabled checks ?
As it is now, it is like "all the checks of the bugprone family except..."

Since there are quite a lot of bugprone- checks enabled now, I think it is best how it is now.

weird... it does not work. Maybe the version of clan-tidy is too old ?

Is it possible to add "all the checks of the bugprone family except..." as a comment in the clang-tidy file?

Ah, yes, that option was introduced after clang-tidy v 20

Co-authored-by: Alexander Sinn <[email protected]>

AlexanderSinn · 2025-12-08T16:56:24Z

The output if it fails is actually quite nice:

 /usr/local/bin/cmake -E create_symlink /home/runner/work/hipace/hipace/build_clang_tidy/bin/hipace.MPI.OMP.SP.LF /home/runner/work/hipace/hipace/build_clang_tidy/bin/hipace
 gmake[2]: Leaving directory '/home/runner/work/hipace/hipace/build_clang_tidy'
 [100%] Built target HiPACE
 gmake[1]: Leaving directory '/home/runner/work/hipace/hipace/build_clang_tidy'
 /usr/local/bin/cmake -E cmake_progress_start /home/runner/work/hipace/hipace/build_clang_tidy/CMakeFiles 0
 
 
 clang-tidy found the following issues:
 
 /home/runner/work/hipace/hipace/src/utils/MultiBuffer.cpp:810:37: warning: unchecked access to optional value [bugprone-unchecked-optional-access]
   810 |             memcpy_to_buffer(slice, bo.m_beam_idcpu[b].value(),
       |                                     ^
 /home/runner/work/hipace/hipace/src/utils/MultiBuffer.cpp:864:39: warning: unchecked access to optional value [bugprone-unchecked-optional-access]
   864 |             memcpy_from_buffer(slice, bo.m_beam_idcpu[b].value(),
       |                                       ^
 
 =============================================
 ##[error]Process completed with exit code 1.

lucafedeli88 · 2025-12-08T17:05:59Z

The output if it fails is actually quite nice:

 /usr/local/bin/cmake -E create_symlink /home/runner/work/hipace/hipace/build_clang_tidy/bin/hipace.MPI.OMP.SP.LF /home/runner/work/hipace/hipace/build_clang_tidy/bin/hipace
 gmake[2]: Leaving directory '/home/runner/work/hipace/hipace/build_clang_tidy'
 [100%] Built target HiPACE
 gmake[1]: Leaving directory '/home/runner/work/hipace/hipace/build_clang_tidy'
 /usr/local/bin/cmake -E cmake_progress_start /home/runner/work/hipace/hipace/build_clang_tidy/CMakeFiles 0
 
 
 clang-tidy found the following issues:
 
 /home/runner/work/hipace/hipace/src/utils/MultiBuffer.cpp:810:37: warning: unchecked access to optional value [bugprone-unchecked-optional-access]
   810 |             memcpy_to_buffer(slice, bo.m_beam_idcpu[b].value(),
       |                                     ^
 /home/runner/work/hipace/hipace/src/utils/MultiBuffer.cpp:864:39: warning: unchecked access to optional value [bugprone-unchecked-optional-access]
   864 |             memcpy_from_buffer(slice, bo.m_beam_idcpu[b].value(),
       |                                       ^
 
 =============================================
 ##[error]Process completed with exit code 1.

Sorry, it was not clear in what I wrote before. The option to silence this check for .value() calls is not available in clang v 18 that is used in CI. They have added it after v 20 . So the only option for now is to use a NOLINT . I am checking that it works before pushing the PR.

lucafedeli88 · 2025-12-08T17:23:19Z

@AlexanderSinn , since I can't disable the warning about the .value() access to an optional type with this clang-tidy version, I've added NOLINT to disable the check for two selected lines.
I've also added a comment to the .clang-tidy configuration file.

lucafedeli88 · 2025-12-08T17:29:33Z

Ok! It seems to work now., @AlexanderSinn .

lucafedeli88 added 3 commits October 24, 2025 14:25

make FFTPoissonSolver's destructor = default

1eb5a10

add tool to run clang-tidy locally

4cad3e6

Merge remote-tracking branch 'origin/development' into add_tools_to_u…

8eb630a

…se_clang_tidy

lucafedeli88 commented Oct 28, 2025

View reviewed changes

tools/runClangTidy.sh Outdated Show resolved Hide resolved

Update tools/runClangTidy.sh

18077c4

lucafedeli88 mentioned this pull request Oct 28, 2025

OpenPMDWriter.cpp & HpMultiGrid.H : enforce clang-tidy rule bugprone-branch-clone #1304

Merged

9 tasks

lucafedeli88 added 2 commits October 29, 2025 23:58

Merge remote-tracking branch 'upstream/development' into add_tools_to…

63e426d

…_use_clang_tidy

Merge remote-tracking branch 'origin/development' into add_tools_to_u…

5e015c9

…se_clang_tidy

lucafedeli88 changed the title ~~Add tools to use clang tidy locally~~ [WIP] Add tools to use clang tidy locally Nov 24, 2025

lucafedeli88 added 17 commits November 24, 2025 11:21

add clang-tidy in CI

a44e221

add missing dependency

8d81667

update runClangTidy script

77e124a

add missing dependency

a169d14

add missing dependency

cc675d9

silence two warnings

8674d76

disable vectorization warnings in CI

99e0b9a

disable vectorization errors also for the linking step

949465f

fix env. variables bug

dafc962

add clearer messages and exit with code 1 if clang-tidy finds issues

6238b6e

check file size

e8b0780

add checks of the cert-* family

0d5d04d

fix bug

8555a3d

only keep cert-err58-cpp for the moment

5826dee

remove cert checks and enable most checks of the bugprone family

542090f

update rules

884c2b5

implement fixes

b05306f

lucafedeli88 mentioned this pull request Nov 26, 2025

HpMultiGrid.H: add default case to switch statements #1306

Closed

9 tasks

lucafedeli88 added 6 commits November 26, 2025 14:23

fix bug introduced in previous commit

f308ac3

fix bug introduced in previous commit

e769987

Merge remote-tracking branch 'origin/development' into add_tools_to_u…

cf3857b

…se_clang_tidy

remove unused phys_const from 2 files

a5b63ae

fix bug

ccda6b6

fix bug

2ff0b94

lucafedeli88 changed the title ~~[WIP] Add tools to use clang tidy locally~~ Add clang tidy CI test & tools to use it locally Nov 26, 2025

lucafedeli88 added 2 commits November 26, 2025 15:25

address issue found with clang-tidy

fccf114

change how the has_value check is done

0af034b

lucafedeli88 mentioned this pull request Nov 27, 2025

Remove unused PhysConst #1317

Merged

9 tasks

Merge remote-tracking branch 'origin/development' into add_tools_to_u…

b4d7e2f

…se_clang_tidy

AlexanderSinn requested changes Dec 8, 2025

View reviewed changes

lucafedeli88 and others added 2 commits December 8, 2025 17:29

Update .github/workflows/clangtidy.yml

5ff5a6b

Co-authored-by: Alexander Sinn <[email protected]>

revert changes in MultiBuffer.cpp

f88aac3

lucafedeli88 force-pushed the add_tools_to_use_clang_tidy branch from 1849513 to 3d70f1f Compare December 8, 2025 16:56

lucafedeli88 added 2 commits December 8, 2025 17:57

revert last change to .clang-tidy

3d70f1f

delete check option

07639ad

lucafedeli88 added 2 commits December 8, 2025 18:07

use NOLINT

e2db756

add comment to clang-tidy configuration file

929df68

lucafedeli88 requested a review from AlexanderSinn December 8, 2025 17:29

AlexanderSinn approved these changes Dec 9, 2025

View reviewed changes

AlexanderSinn merged commit 9b90e06 into Hi-PACE:development Dec 9, 2025
10 of 11 checks passed

Conversation

lucafedeli88 commented Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

lucafedeli88 commented Nov 20, 2025

Uh oh!

AlexanderSinn commented Nov 20, 2025

Uh oh!

lucafedeli88 commented Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lucafedeli88 commented Nov 26, 2025

Uh oh!

lucafedeli88 commented Dec 5, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexanderSinn commented Dec 8, 2025

Uh oh!

lucafedeli88 commented Dec 8, 2025

Uh oh!

lucafedeli88 commented Dec 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lucafedeli88 commented Dec 8, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Comments

lucafedeli88 commented Oct 27, 2025 •

edited

Loading

lucafedeli88 commented Nov 21, 2025 •

edited

Loading

lucafedeli88 commented Dec 8, 2025 •

edited

Loading