Skip to content

Clang tidy autofix - #3374

Open
Lestropie wants to merge 31 commits into
devfrom
clang-tidy-autofix
Open

Clang tidy autofix#3374
Lestropie wants to merge 31 commits into
devfrom
clang-tidy-autofix

Conversation

@Lestropie

@Lestropie Lestropie commented May 28, 2026

Copy link
Copy Markdown
Member

Downstream of #3368.

Draft PR for now; I plan to at some point review individual commits myself before asking for feedback, but it's not a high priority right now, I just wanted it committed and lodged to flush it from my in-progress task list.


As first exemplified in #2829, clang-tidy has the capability---for some of its checks at least---to automatically apply the prescribed correction corresponding to the issue detected. In my limited experience it can be a bit hit-and-miss as to whether it gets things right or not. But combined with generative AI to clean up any compilation errors, it's potentially quite powerful for not a whole lot of human time investment.

#3368 introduced the capability to split clang-tidy detection capabilities between what is merely reported in an auto-generated PR review (which we've had running for a little while now), and enforcement of certain checks in PR content (which should ideally be enforced across the whole code base before commencing enforcement on PR content).

What I've done here is interrogated which clang-tidy checks both have the autofix capability, and would potentially yield improvement to code quality / mitigation of bugs. These were run in rank order, re-establishing compilation each time, and also adding that specific check to the enforcement clang-tidy CI check. These all ran through with pretty minimal time investment on my part (thus far; code review is obviously a different question, though passing all C++ tests should give some confidence).

Edit: Manual review completed.

Lestropie added 19 commits May 28, 2026 19:35
Apply clang-tidy-18 performance-* autofixes repository-wide and add
performance-* to the enforced check set in .clang-tidy-enforce.

Manual corrections accompanying the autofixes:
- Synchronise noexcept specifiers on out-of-line move constructor and
  move assignment definitions whose in-class declarations gained noexcept
  (connectome Edge / Node::Mesh / FileDataVector, ROI_UndoEntry).
- Convert parameters that the autofix turned into const std::string& into
  std::string_view to satisfy the project syntax requirement, constructing
  a std::string only where an owning string is needed.

Generated-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Applies const-correctness improvements suggested by clang-tidy-18's
misc-const-correctness check across the codebase (~266 files), plus the
manual corrections required for the tree to build.

The check itself is NOT enforced: it is added to .clang-tidy-enforce but
commented out. Under WarningsAsErrors: '*' it cannot pass, because it
conflicts with the codebase's ThreadedLoop functor idiom -- functor
objects passed into ThreadedLoop().run(...) expose a non-const
operator(), so the compiler requires them non-const, but clang-tidy
cannot see through the template forwarding and perpetually demands const
on them (~20 sites).

Generated-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Applies clang-tidy-18 readability-implicit-bool-conversion autofixes across
the C++ codebase and adds the check to .clang-tidy-enforce. This aligns with
the house rule of comparing integer/floating-point/pointer values explicitly
rather than relying on implicit boolean casts.

Generated-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Generated-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous commit added performance-* to .clang-tidy-enforce, but the
gate did not actually pass: ~2160 residual diagnostics remained because
several performance sub-checks have no autofix (or an autofix that
conflicts with project rules), so a clang-tidy enforce run under
WarningsAsErrors: '*' would fail despite a green build.

Excludes the sub-checks with large residuals or unsafe/rule-conflicting
fixes, keeping the rest enforced and clean:
  - performance-enum-size                  (1330; no autofix, low value)
  - performance-unnecessary-value-param    (420; autofix yields
                                            const std::string& which
                                            check_syntax rejects in
                                            favour of std::string_view)
  - performance-move-constructor-init      (294)
  - performance-no-int-to-ptr              (46)
  - performance-type-promotion-in-math-fn  (22)
  - performance-no-automatic-move          (8)

Applies the remaining auto-fixable performance fixes (for-range-copy,
faster-string-find, avoid-endl, unnecessary-copy-initialization,
inefficient-vector-operation, noexcept-move-constructor). A full
enforce-config run now reports zero performance-* diagnostics.

Generated-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Checks field is a YAML folded scalar: lines are concatenated with
spaces and the result is split on commas. The `#`-prefixed comment lines
introduced for misc-const-correctness and the performance-* exclusions
had no trailing comma, so each comment token absorbed the disable entry
on the next line. As a result `-bugprone-easily-swappable-parameters`
and `-performance-enum-size` were NOT applied (clang-tidy --list-checks
showed both as enabled), producing ~6000 spurious would-be errors.

Add a trailing comma to each comment line so comma-splitting isolates
the (inert, non-matching) comment token from the real disable that
follows it.

Generated-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Generated-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Generated-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Generated-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Generated-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Generated-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Generated-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Apply modernize-make-unique autofixes converting
std::unique_ptr<T>(new T(args)) to std::make_unique<T>(args),
and enable the check in .clang-tidy-enforce.

Three sites in cpp/cmd/mrcalc.cpp use MR::copy_ptr (not std::unique_ptr)
and one site in cpp/core/file/npy.cpp passes a braced-init-list that
cannot be perfect-forwarded; these retain .reset(new ...) with targeted
NOLINT(modernize-make-unique) annotations.

Generated-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Generated-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Generated-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Generated-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Generated-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Generated-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Generated-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Scanned the C++ files modified by commit 3410f8d (the
misc-const-correctness clang-tidy pass) and relocated every East-const
placement (TYPE const VAR) to West-const (const TYPE VAR) for
non-pointer variable declarations, where both forms are semantically
equivalent but the West-const style is consistent with the rest of the
codebase. Const-pointer declarations (TYPE* const VAR) and member
function qualifiers (fn() const) were correctly left untouched. One
inline-block declaration in tckedit.cpp that the regex missed was
corrected manually. A spurious transformation in slangcodegen.cpp,
where const appeared inside a string literal, was detected and reverted.

Session prompts:
1. > Examine content of commit 3410f8d.
   > Anywhere that a "const" qualifier has been added to a variable that
   > is not a pointer, move the "const" keyword from the right side of
   > the type specifier to the left.

Generated-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Introduce a make_copyptr<>() helper in cpp/core/memory.h that constructs a
copy_ptr<T> owning a newly-allocated T, mirroring the std::make_unique<>()
and std::make_shared<>() idiom. The variadic template perfect-forwards its
arguments into the T constructor and wraps the result via the explicit
copy_ptr(T*) constructor, satisfying the existing call sites in mrcalc.cpp.
Also adds the <utility> include required for std::forward.

Session prompts:
1. > Find the git branch on this repository where, within file
   > cpp/core/memory.h, a function make_copyptr<>() has been created,
   > mimicking std::make_unique<>() and std::make_shared<>().
2. > Add the make_copyptr<>() definition to memory.h

Generated-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Conflicts:
	.clang-tidy-enforce
	cpp/cmd/amp2response.cpp
	cpp/cmd/dwidenoise.cpp
	cpp/cmd/mrcalc.cpp
	cpp/cmd/mrcolour.cpp
	cpp/cmd/mredit.cpp
	cpp/cmd/mrgrid.cpp
	cpp/cmd/mrmetric.cpp
	cpp/cmd/mrstats.cpp
	cpp/cmd/sh2peaks.cpp
	cpp/cmd/tcksample.cpp
	cpp/cmd/tsfinfo.cpp
	cpp/core/dwi/tractography/SIFT/proc_mask.cpp
	cpp/core/dwi/tractography/algorithms/tensor_prob.h
	cpp/core/dwi/tractography/seeding/basic.cpp
	cpp/core/dwi/tractography/seeding/gmwmi.cpp
	cpp/core/dwi/tractography/seeding/list.cpp
	cpp/core/dwi/tractography/tracking/method.cpp
	cpp/core/file/config.cpp
	cpp/core/math/SH.h
	cpp/core/registration/linear.h
	cpp/core/stats.h
	cpp/core/surface/algo/mesh2image.cpp
	cpp/gui/dwi/render_frame.cpp
	cpp/gui/mrview/mode/volume.cpp
	cpp/gui/mrview/sync/interprocesscommunicator.cpp
	cpp/gui/mrview/tool/connectome/file_data_vector.cpp
	cpp/gui/mrview/tool/roi_editor/undoentry.cpp
	cpp/gui/mrview/tool/tractography/track_scalar_file.cpp
	cpp/gui/mrview/tool/tractography/tractography.cpp
	cpp/gui/mrview/window.cpp
Prompt:
> File "clang_tidy.txt" contains a dump from execution of the "clang-tidy" tool across the cpp/ directory. It was instructed to specifically apply the "readability-isolate-declaration" check. It was however unable to perform an automatic fix of this pattern. Find all instances of that warning in the clang-tidy log file, and apply the corresponding fix (one line per variable declaration) yourself.

Generated-by: Claude Opus 4.8 <noreply@anthropic.com>
@Lestropie
Lestropie force-pushed the clang-tidy-autofix branch from 5b92150 to 875bc2e Compare June 19, 2026 01:43
- Fix new initialisation style for std::vector class member variables being treated as a std::initializer_list rather than constructor inputs leading to incorrect smoothing & buffer overflow in image smoothing.
- Fix parsing of multi-file numbered image inputs due to adoption of std::find() rather than manual loop.
- Fix excess space in mrstats field outputs due to adoption of normalized loop.
@Lestropie
Lestropie marked this pull request as ready for review June 20, 2026 10:37
@Lestropie

Copy link
Copy Markdown
Member Author

This work should I think be done. There are however outstanding questions to be posed to @MRtrix3/mrtrix3-devs:

  1. Adoption vs. enforcement.
    For some checks, it's quite possible that while modifying the existing code base through automated means is a net benefit, enforcement of those same checks upon devs and external contributors alike might be considered excessive. So the code changes could be preserved but the checks removed from the enforcement configuration file (which now does pre-commit hook as well as CI).

  2. Set of checks.
    Are there any modifications here that any devs outright disagree with and would prefer to be omitted from the changeset? Or knowledge & preference for one that I've not yet included here?

The ones I'd highlight as possible to receive exception:

  1. @jdtournier seems to like implicit bool conversions. However I'm increasingly disliking them. Makes it impossible to tell from a conditional statement what is intrinsically a bool. And it's bug-prone; this check is what exposed Fix image gradients w.r.t. scanner for non-1mm & anisotropic voxels #3392, which has potentially been detrimental to non-linear registration performance from the outset.

  2. The auto check I don't always agree with. I would typically only use it for very long and/or complex type names. However the determining factor for clang-tidy is whether the type can be inferred from the statement. Which means that if one does e.g. static_cast<size_t>(std::round(value)), it will insist on using auto.

  3. [[nodiscard]] can take up quite a bit of space when navigating library headers. It is however a bug detection tool, which makes it hard to exclude. FORCE_INLINE is similarly distracting in some header files and is probably no longer needed (or indeed detrimental)...

  4. The readability-isolate-declaration check I added late. I definitely get caught out at times missing subtle multi-declarations; and sometimes one will have a default initialisation and the other won't, which becomes difficult to spot. It feels counter-intuitive to enforce a column line width limit for readability and side-by-side comparison, but then obfuscate declarations mid-line. I've started writing all code like this some time ago I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant