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

Require clang-tidy in CI #1524

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from

Conversation

esseivaju
Copy link
Contributor

We've had a clang-tidy job for a while, but it wasn't required to succeed for the CI pipeline to pass. This PR makes it required: if we ever want to require it, the sooner the easier it will be.

I silenced most of the remaining warnings as they were inconsequential, except for a few suspicious cases where I fixed the warning, if they were fine they'd warrant a comment. In particular, fix a potential memory leak in CelerOpticalPhysics if a process is inactive.

@esseivaju esseivaju added documentation Documentation, examples, tests, and CI core Software engineering infrastructure labels Nov 28, 2024
Copy link

github-actions bot commented Nov 28, 2024

Test summary

 3 844 files   5 936 suites   4m 7s ⏱️
 1 597 tests  1 569 ✅ 28 💤 0 ❌
19 790 runs  19 715 ✅ 75 💤 0 ❌

Results for commit 81d0374.

♻️ This comment has been updated with latest results.

Copy link
Member

@sethrj sethrj left a comment

Choose a reason for hiding this comment

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

Thanks @esseivaju ! It's nice to finally have this in place 😄

I think given the number of "false" positives for cppcoreguidelines-rvalue-reference-param-not-moved and cppcoreguidelines-missing-std-forward we should just disable those.

.github/workflows/build-spack.yml Show resolved Hide resolved
.github/workflows/build-spack.yml Show resolved Hide resolved
.github/workflows/build-spack.yml Outdated Show resolved Hide resolved
src/celeritas/ext/detail/CelerOpticalPhysics.cc Outdated Show resolved Hide resolved
src/celeritas/ext/detail/CelerOpticalPhysics.cc Outdated Show resolved Hide resolved
src/geocel/GeoVolumeFinder.cc Outdated Show resolved Hide resolved
src/orange/g4org/SolidConverter.cc Outdated Show resolved Hide resolved
src/orange/surf/GeneralQuadric.cc Show resolved Hide resolved
src/orange/surf/SimpleQuadric.cc Show resolved Hide resolved
app/celer-dump-data.cc Show resolved Hide resolved
@esseivaju
Copy link
Contributor Author

I think given the number of "false" positives for cppcoreguidelines-rvalue-reference-param-not-moved and cppcoreguidelines-missing-std-forward we should just disable those.

I enabled AllowPartialMove which makes the check more lenient. There's only a handful left now. Since this is an unusual way of passing parameters (per F.15), I think these checks could be useful, it makes you think twice about it, and future readers know that it was deliberate and not an oversight.

app/celer-dump-data.cc Show resolved Hide resolved
src/corecel/sys/Stream.cc Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Software engineering infrastructure documentation Documentation, examples, tests, and CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants