Skip to content

Commit

Permalink
ci: Add clang-tidy check for &&, || and ! instead of and, or and not (#…
Browse files Browse the repository at this point in the history
  • Loading branch information
paulgessinger authored Oct 31, 2023
1 parent fbc3bd8 commit 1f6dd58
Show file tree
Hide file tree
Showing 320 changed files with 1,366 additions and 1,205 deletions.
5 changes: 4 additions & 1 deletion .clang-tidy
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
---
Checks: '-*,readability-container-size-empty,readability-implicit-bool-cast,readability-implicit-bool-conversion,modernize-use-equals-default,modernize-use-override,modernize-use-using,readability-braces-around-statements,modernize-use-nullptr,performance-move-const-arg,cppcoreguidelines-pro-type-member-init,cppcoreguidelines-init-variables,clang-analyzer-optin.cplusplus.UninitializedObject'
Checks: '-*,readability-container-size-empty,readability-implicit-bool-cast,readability-implicit-bool-conversion,modernize-use-equals-default,modernize-use-override,modernize-use-using,readability-braces-around-statements,modernize-use-nullptr,performance-move-const-arg,cppcoreguidelines-pro-type-member-init,cppcoreguidelines-init-variables,clang-analyzer-optin.cplusplus.UninitializedObject,readability-operators-representation'
HeaderFilterRegex: '.*(?<!nlohmann\/json)\.(hpp|cpp|ipp)$'
AnalyzeTemporaryDtors: true
CheckOptions:
readability-operators-representation.BinaryOperators: '&&;&=;&;|;~;!;!=;||;|=;^;^='
readability-operators-representation.OverloadedOperators: '&&;&=;&;|;~;!;!=;||;|=;^;^='
10 changes: 5 additions & 5 deletions .gitlab-ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,11 @@ clang_tidy:
apt-get update
&& apt-get install -y g++-8 libstdc++-8-dev parallel software-properties-common
&& curl -L https://apt.llvm.org/llvm-snapshot.gpg.key | apt-key add
&& add-apt-repository 'deb http://apt.llvm.org/focal/ llvm-toolchain-focal-16 main'
&& apt-get install -y clang-16 clang-tidy-16
&& ln -s /usr/bin/clang++-16 /usr/bin/clang++
&& ln -s /usr/bin/clang-16 /usr/bin/clang
&& ln -s /usr/bin/clang-tidy-16 /usr/bin/clang-tidy
&& add-apt-repository 'deb http://apt.llvm.org/focal/ llvm-toolchain-focal-17 main'
&& apt-get install -y clang-17 clang-tidy-17
&& ln -s /usr/bin/clang++-17 /usr/bin/clang++
&& ln -s /usr/bin/clang-17 /usr/bin/clang
&& ln -s /usr/bin/clang-tidy-17 /usr/bin/clang-tidy
&& mkdir -p /opt/lib/gcc/x86_64-linux-gnu
&& ln -s /usr/lib/gcc/x86_64-linux-gnu/8/ /opt/lib/gcc/x86_64-linux-gnu/
&& clang++ --gcc-toolchain=/opt -v
Expand Down
10 changes: 5 additions & 5 deletions Alignment/include/ActsAlignment/Kernel/Alignment.ipp
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ ActsAlignment::Alignment<fitter_t>::evaluateTrackAlignmentState(
// Perform the fit
auto fitRes = m_fitter.fit(begin, end, sParameters, fitOptions, tracks);

if (not fitRes.ok()) {
if (!fitRes.ok()) {
ACTS_WARNING("Fit failure");
return fitRes.error();
}
Expand Down Expand Up @@ -89,7 +89,7 @@ void ActsAlignment::Alignment<fitter_t>::calculateAlignmentParameters(
auto evaluateRes = evaluateTrackAlignmentState(
fitOptions.geoContext, sourcelinks, sParameters,
fitOptionsWithRefSurface, alignResult.idxedAlignSurfaces, alignMask);
if (not evaluateRes.ok()) {
if (!evaluateRes.ok()) {
ACTS_DEBUG("Evaluation of alignment state for track " << iTraj
<< " failed");
continue;
Expand Down Expand Up @@ -200,7 +200,7 @@ ActsAlignment::Alignment<fitter_t>::updateAlignmentParameters(
<< deltaAlignmentParam);
bool updated = alignedTransformUpdater(alignedDetElements.at(index), gctx,
newTransform);
if (not updated) {
if (!updated) {
ACTS_ERROR("Update alignment parameters for detector element failed");
return AlignmentError::AlignmentParametersUpdateFailure;
}
Expand Down Expand Up @@ -289,15 +289,15 @@ ActsAlignment::Alignment<fitter_t>::align(
auto updateRes = updateAlignmentParameters(
alignOptions.fitOptions.geoContext, alignOptions.alignedDetElements,
alignOptions.alignedTransformUpdater, alignResult);
if (not updateRes.ok()) {
if (!updateRes.ok()) {
ACTS_ERROR("Update alignment parameters failed: " << updateRes.error());
return updateRes.error();
}
alignmentParametersUpdated = true;
} // end of all iterations

// Alignment failure if not converged
if (not converged) {
if (!converged) {
ACTS_ERROR("Alignment is not converged.");
alignResult.result = AlignmentError::ConvergeFailure;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ TrackAlignmentState trackAlignmentState(

// Only measurement states matter (we can't align non-measurement states,
// no?)
if (not ts.typeFlags().test(TrackStateFlag::MeasurementFlag)) {
if (!ts.typeFlags().test(TrackStateFlag::MeasurementFlag)) {
return true;
}
// Check if the reference surface is to be aligned
Expand Down
12 changes: 6 additions & 6 deletions Alignment/src/Kernel/detail/AlignmentEngine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,22 +13,22 @@ namespace detail {

void resetAlignmentDerivative(Acts::AlignmentToBoundMatrix& alignToBound,
AlignmentMask mask) {
if (not ACTS_CHECK_BIT(mask, AlignmentMask::Center0)) {
if (!ACTS_CHECK_BIT(mask, AlignmentMask::Center0)) {
alignToBound.col(Acts::eAlignmentCenter0) = Acts::AlignmentVector::Zero();
}
if (not ACTS_CHECK_BIT(mask, AlignmentMask::Center1)) {
if (!ACTS_CHECK_BIT(mask, AlignmentMask::Center1)) {
alignToBound.col(Acts::eAlignmentCenter1) = Acts::AlignmentVector::Zero();
}
if (not ACTS_CHECK_BIT(mask, AlignmentMask::Center2)) {
if (!ACTS_CHECK_BIT(mask, AlignmentMask::Center2)) {
alignToBound.col(Acts::eAlignmentCenter2) = Acts::AlignmentVector::Zero();
}
if (not ACTS_CHECK_BIT(mask, AlignmentMask::Rotation0)) {
if (!ACTS_CHECK_BIT(mask, AlignmentMask::Rotation0)) {
alignToBound.col(Acts::eAlignmentRotation0) = Acts::AlignmentVector::Zero();
}
if (not ACTS_CHECK_BIT(mask, AlignmentMask::Rotation1)) {
if (!ACTS_CHECK_BIT(mask, AlignmentMask::Rotation1)) {
alignToBound.col(Acts::eAlignmentRotation1) = Acts::AlignmentVector::Zero();
}
if (not ACTS_CHECK_BIT(mask, AlignmentMask::Rotation2)) {
if (!ACTS_CHECK_BIT(mask, AlignmentMask::Rotation2)) {
alignToBound.col(Acts::eAlignmentRotation2) = Acts::AlignmentVector::Zero();
}
}
Expand Down
1 change: 1 addition & 0 deletions CI/clang_tidy/limits.yml
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,4 @@ limits:
"cppcoreguidelines-init-variables": 0
"clang-analyzer-optin.cplusplus.UninitializedObject": 0
"clang-diagnostic-error": 0
"readability-operators-representation": 0
169 changes: 169 additions & 0 deletions CI/clang_tidy/run_clang_tidy.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,169 @@
#!/usr/bin/env python3
import argparse
from concurrent.futures import ThreadPoolExecutor, as_completed
from multiprocessing import cpu_count
import subprocess
from subprocess import check_call, check_output, CalledProcessError
from pathlib import Path
import json
import re
import sys
import os
import threading

import rich.console
import rich.progress
import rich.panel
import rich.live
import rich.text
import rich.table
import rich.rule
import rich.spinner


def which(cmd: str):
try:
return check_output(["command", "-v", cmd]).decode().strip()
except CalledProcessError:
return None


def main():
p = argparse.ArgumentParser()
p.add_argument("--clang-tidy", default=which("clang-tidy"))
p.add_argument("--clang-format", default=which("clang-format"))
p.add_argument("--jobs", "-j", type=int, default=cpu_count())
p.add_argument("--fix", action="store_true")
p.add_argument("--include", action="append", default=[])
p.add_argument("--exclude", action="append", default=[])
p.add_argument("--ignore-compiler-errors", action="store_true")
p.add_argument("build", type=Path)
p.add_argument("source", type=Path)

args = p.parse_args()

assert args.clang_tidy is not None, "clang-tidy not found"
assert args.clang_format is not None, "clang-format not found"

args.include = [re.compile(f) for f in args.include]
args.exclude = [re.compile(f) for f in args.exclude]

check_call([args.clang_tidy, "--version"], stdout=subprocess.DEVNULL)
check_call([args.clang_format, "--version"], stdout=subprocess.DEVNULL)

assert (
args.build.exists() and args.build.is_dir()
), f"{args.build} is not a directory"
assert (
args.source.exists() and args.source.is_dir()
), f"{args.source} is not a directory"

with (args.build / "compile_commands.json").open() as f:
compilation_database = json.load(f)

futures = []
files = []
active_files = {}
active_file_lock = threading.Lock()

def run(file: Path):
with active_file_lock:
active_files[threading.current_thread().ident] = file
cmd = [args.clang_tidy, "-p", args.build, file]
if args.fix:
cmd.append("-fix")

try:
out = check_output(cmd, stderr=subprocess.STDOUT).decode().strip()
error = False
except CalledProcessError as e:
out = e.output.decode().strip()
if args.ignore_compiler_errors and "Found compiler error(s)." in out:
out = "Found compiler error(s)."
error = False
else:
error = True
finally:
with active_file_lock:
active_files[threading.current_thread().ident] = None
return file, out, error

for dirpath, _, filenames in os.walk(args.source):
dirpath = Path(dirpath)
for file in filenames:
file = dirpath / file
if (
file.suffix in (".hpp", ".cpp", ".ipp")
and (
len(args.include) == 0
or any(flt.match(str(file)) for flt in args.include)
)
and not any(flt.match(str(file)) for flt in args.exclude)
):
files.append(file)

with ThreadPoolExecutor(args.jobs) as tp:
for file in files:
assert file.exists(), f"{file} does not exist"
futures.append(tp.submit(run, file))

error = False

console = rich.console.Console()

prog = rich.progress.Progress()
log = []

def make_display():
t = rich.table.Table.grid()
t.add_column()
t.add_column()
with active_file_lock:
for f in active_files.values():
if f is None:
t.add_row("")
else:
t.add_row(rich.spinner.Spinner("dots", style="green"), f" {f}")

ot = rich.table.Table.grid(expand=True)
ot.add_column(ratio=1)
ot.add_column(ratio=1)

def emoji(err):
return ":red_circle:" if err else ":green_circle:"

ot.add_row(
t,
rich.console.Group(
*[f"{emoji(err)} {line}" for err, line in log[-args.jobs :]]
),
)

return rich.console.Group(rich.rule.Rule(), ot, prog)

task = prog.add_task("Running clang-tidy", total=len(futures))

with rich.live.Live(
make_display(), console=console, refresh_per_second=20, transient=False
) as live:
for f in as_completed(futures):
file, result, this_error = f.result()
log.append((this_error, file))
error = this_error or error
console.print(
rich.panel.Panel(
result, title=str(file), style="red" if this_error else ""
)
)
prog.advance(task)
live.update(make_display())
live.refresh()

if error:
return 1
else:
return 0


if __name__ == "__main__":
sys.exit(main())
4 changes: 2 additions & 2 deletions Core/include/Acts/Detector/detail/IndexedGridFiller.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ std::set<typename grid_type::index_t> localIndices(
throw std::runtime_error("IndexedSurfaceGridFiller: no query point given.");
}

if (not expansion.empty() and expansion.size() != grid_type::DIM) {
if (!expansion.empty() && expansion.size() != grid_type::DIM) {
throw std::runtime_error(
"IndexedSurfaceGridFiller: wrong dimension of bin expansion given.");
}
Expand Down Expand Up @@ -222,7 +222,7 @@ struct IndexedGridFiller {
}

// Assign the indices into all
if (not aToAll.empty()) {
if (!aToAll.empty()) {
assignToAll(iGrid, aToAll);
}
}
Expand Down
4 changes: 2 additions & 2 deletions Core/include/Acts/Digitization/DigitizationSourceLink.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -47,12 +47,12 @@ class DigitizationSourceLink final {

inline bool operator==(const DigitizationSourceLink& lhs,
const DigitizationSourceLink& rhs) {
return (lhs.geometryId() == rhs.geometryId()) and
return (lhs.geometryId() == rhs.geometryId()) &&
(lhs.indices() == rhs.indices());
}
inline bool operator!=(const DigitizationSourceLink& lhs,
const DigitizationSourceLink& rhs) {
return not(lhs == rhs);
return !(lhs == rhs);
}

} // namespace Acts
10 changes: 5 additions & 5 deletions Core/include/Acts/EventData/Charge.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ struct Neutral {
///
/// This constructor is only provided to allow consistent construction.
constexpr Neutral(float absQ) noexcept {
assert((absQ == 0) and "Input charge must be zero");
assert((absQ == 0) && "Input charge must be zero");
(void)absQ;
}

Expand All @@ -80,7 +80,7 @@ struct Neutral {

template <typename P, typename Q>
constexpr auto qOverP(P momentum, Q signedQ) const noexcept {
assert((signedQ != 0) and "charge must be 0");
assert((signedQ != 0) && "charge must be 0");
(void)signedQ;
return 1.0f / momentum;
}
Expand All @@ -106,7 +106,7 @@ struct SinglyCharged {
///
/// This constructor is only provided to allow consistent construction.
constexpr SinglyCharged(float absQ) noexcept {
assert((absQ == UnitConstants::e) and "Input charge magnitude must be e");
assert((absQ == UnitConstants::e) && "Input charge magnitude must be e");
(void)absQ;
}

Expand Down Expand Up @@ -151,7 +151,7 @@ class NonNeutralCharge {
public:
/// Construct with the magnitude of the input charge.
constexpr NonNeutralCharge(float absQ) noexcept : m_absQ{absQ} {
assert((0 < absQ) and "Input charge magnitude must be positive");
assert((0 < absQ) && "Input charge magnitude must be positive");
}
constexpr NonNeutralCharge(SinglyCharged /*unused*/) noexcept
: m_absQ{UnitConstants::e} {}
Expand Down Expand Up @@ -200,7 +200,7 @@ class AnyCharge {
public:
/// Construct with the magnitude of the input charge.
constexpr AnyCharge(float absQ) noexcept : m_absQ{absQ} {
assert((0 <= absQ) and "Input charge magnitude must be zero or positive");
assert((0 <= absQ) && "Input charge magnitude must be zero or positive");
}
constexpr AnyCharge(SinglyCharged /*unused*/) noexcept
: m_absQ{UnitConstants::e} {}
Expand Down
10 changes: 5 additions & 5 deletions Core/include/Acts/EventData/GenericBoundTrackParameters.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ class GenericBoundTrackParameters {
const std::optional<CovarianceMatrix>& covariance() const { return m_cov; }
/// Covariance matrix of the spatial impact parameters (i.e., of d0 and z0)
std::optional<ActsSquareMatrix<2>> spatialImpactParameterCovariance() const {
if (not m_cov.has_value()) {
if (!m_cov.has_value()) {
return std::nullopt;
}

Expand All @@ -146,7 +146,7 @@ class GenericBoundTrackParameters {
/// Covariance matrix of the spatial and temporal impact parameters (i.e., of
/// d0, z0, and t)
std::optional<ActsSquareMatrix<3>> impactParameterCovariance() const {
if (not m_cov.has_value()) {
if (!m_cov.has_value()) {
return std::nullopt;
}

Expand Down Expand Up @@ -278,14 +278,14 @@ class GenericBoundTrackParameters {
/// want and we might decided that we will remove this in the future.
friend bool operator==(const GenericBoundTrackParameters& lhs,
const GenericBoundTrackParameters& rhs) {
return (lhs.m_params == rhs.m_params) and (lhs.m_cov == rhs.m_cov) and
(lhs.m_surface == rhs.m_surface) and
return (lhs.m_params == rhs.m_params) && (lhs.m_cov == rhs.m_cov) &&
(lhs.m_surface == rhs.m_surface) &&
(lhs.m_particleHypothesis == rhs.m_particleHypothesis);
}
/// Compare two bound track parameters for bitwise in-equality.
friend bool operator!=(const GenericBoundTrackParameters& lhs,
const GenericBoundTrackParameters& rhs) {
return not(lhs == rhs);
return !(lhs == rhs);
}
/// Print information to the output stream.
friend std::ostream& operator<<(std::ostream& os,
Expand Down
Loading

0 comments on commit 1f6dd58

Please sign in to comment.