Clang-tidy enforcement (rebased)#3368
Open
Lestropie wants to merge 15 commits into
Open
Conversation
Add clang-tidy enforcement via pre-commit hook and blocking PR workflow Replace clang-tidy-review action with direct clang-tidy enforcement Session prompts: 1. > Modify the utilisation of the "clang-tidy" tool within this project. > Currently it is utilised only via a GitHub workflow that executes > clang-tidy on the content of pull requests and then submits the > generated content as a review. Clang-tidy should also be used in two > more ways: firstly, as a pre-commit hook on the content of commits; > secondly, as a workflow check with the capacity to block the merging > of Pull Requests. These two new utilisations however require use of an > alternative configuration file provided to clang-tidy that enforces > only a subset of checks. 2. > The "clang-tidy-enforce" GitHub Action is currently posting a response > in the respective Pull Request, and a successful return code despite the > presence of violations in canary function cpp/cmd/linttest.cpp. Modify > the Action so that it does not post to the Pull Request, and the return > code reflects the presence or absence of violations of the enforcement > clang-tidy configuration. Generated-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Rename pre-existing "clang-tidy" -> "clang-tidy-review" Remove clang-tidy enforcement pre-commit hook: to run clang-tidy reliably requires generation of a compilation commands JSON file, which cannot be expected in the context of a pre-commit hook. clang-tidy-enforce Action: Try to suppress 3rd-party violations More attempted changes to get clang-tidy enforcement action working: - Update clang-tidy version to 19 in order to allow use of ExcludeHeaderFilterRegex / --exclude-header-filter=. - Remove negative lookaheads from regexes (not supported in LLVM regex engine). - Try double-escaping forward-slashes in YAML file.
Session prompts:
1. > Create a central location for MRtrix C++ code to access environment
> variables. It should abstract away the C-sting return of
> std::getenv() and return a std::optional<std::string>. It must also
> be made thread-safe. Choose an appropriate location given the
> filesystem arrangement and class / file naming conventions.
2. > For all uses of std::getenv in the code base, replace with a call
> to MR::get_env().
3. > Augment MR::get_env() with an alternative function that additionally
> takes as input a default value. If the environment variable is
> absent, then the function returns the default value. Since a value
> is always returned in this instance, std::optional<> should not be
> used. If the provided default value is of a type other than
> std::string, then any environment variable content should be cast to
> that type via MR::to<> function to return. For all uses of
> MR::get_env() in the repository, if an absence of an environment
> variable results in use of some other value, replace the code with
> the new function that takes that default value as the second input
> argument.
4. > Define a centralised location in the C++ code base where, instead of
> calling strerror() directly, code will instead invoke a custom function
> that generates an internal C string buffer, runs the re-entrant version
> strerror_r(), and returns the result as a std::string. The availability
> of the strerror_r() function needs to be queried during cmake
> configuration. If not present, that function must instead acquire a
> mutex lock while the strerror() function is called, but still return a
> std::string.
5. > Rename function to MR::C_strerror. Migrate all existing strerror()
> calls to use that function. Implement a regex search within the
> "check_syntax" script to ensure that direct invocations of strerror()
> are no longer present and cannot be added in the future, providing user
> instruction to instead use MR::C_strerror().
> Also add a second regex within "check_syntax" to ensure that function
> std::getenv() is no longer utilised and will not be in the future,
> providing user instruction to instead use MR::get_env().
6. > Find all usages of C function "rand()" in the C++ code. Replace with
> corresponding functionality in the <random> STL. In all circumstances,
> determine the most suitable way to ensure thread-safety in that context
> and then implement.
7. > Add regex expressions and user feedback messages to the "check_syntax"
> script to ensure that no reference to srand() or rand() appear in the
> code base.
8. > clang-tidy reports error "bugprone-exception-escape" for "int main()"
> in cpp/core/command.h. It however does not specify from where an
> exception may be raised and uncaught, or the nature of such an
> exception (eg. MR::Exception from cpp/core/exception.h or something
> from std namespace). Investigate possible unhanded exceptions from
> this function and propose rectifications.
9. > Modify the version mismatch check to avoid the use of MR::Exception
> using noexcept functions so that it can appear before the try block.
10. > tackle the remaining escape paths
11. > Can the VT100 terminal symbols used to colour terminal messages be
> applied to the functions that write to stderr within "int main()"
> without violating noexcept?
12. > Apply the same set of changes to function R_main() in
> cpp/core/command.h.
13. > 1. Modify R_main() to use REprintf() with the requisite scaffolding
> to ensure noexcept.
> 2. Modify try-catch structure of R_main() and main() so that once
> MR::App::NAME has been populated, it is included in the terminal
> output, with application of text colouring consistent with the
> rest of MRtrix3.
Generated-by: Claude Sonnet 4.6 <noreply@anthropic.com>
This was referenced May 22, 2026
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Add an INTERFACE wrapper target (mrtrix::eigen) in cmake/Dependencies.cmake that re-exposes Eigen's include directories as SYSTEM includes, so that the project's high warning level and -Werror no longer flag diagnostics originating from within Eigen headers. The wrapper still links Eigen3::Eigen transitively to preserve any upstream compile features and definitions, and works uniformly across both the system-Eigen and FetchContent paths. Consumers in cpp/core and cpp/cmd now link mrtrix::eigen instead of Eigen3::Eigen directly. Verified that the configured build emits -isystem for the Eigen include path. Session prompts: 1. > Is it appropriate to provide header-only libraries such as Eigen3 and > nlohmann_json to cmake function target_link_libraries()? 2. > What about if one would prefer to flag Eigen as SYSTEM such that it is > included in the compilation path via -isystem? 3. > Implement option 3. Generated-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Also includes new check_syntax check to look for C-style casts to void*.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Lestropie
added a commit
that referenced
this pull request
May 24, 2026
Lestropie
force-pushed
the
clang-tidy-rebase
branch
from
1188ec6 to
9bd6cdc
Compare
Lestropie
added a commit
that referenced
this pull request
May 24, 2026
Lestropie
added a commit
that referenced
this pull request
May 24, 2026
Lestropie
force-pushed
the
clang-tidy-rebase
branch
from
3c2009c to
258aeaa
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Lestropie
force-pushed
the
clang-tidy-rebase
branch
from
258aeaa to
3ee0fa1
Compare
Ad additional check has been added to "check_syntax" that ensures that the custom Eigen plugins---previously specified in cpp/core/types.h, now in new file cpp/core/eigen_plugins/eigen_plugins.h---are specified before any Eigen headers are loaded. This should protect against modifications that compile locally but fail in certain CI environments.
Lestropie
force-pushed
the
clang-tidy-rebase
branch
from
3ee0fa1 to
f27a40b
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Lestropie
force-pushed
the
clang-tidy-rebase
branch
from
a875881 to
4575131
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
Lestropie
added a commit
that referenced
this pull request
May 24, 2026
Lestropie
force-pushed
the
clang-tidy-rebase
branch
from
e44236b to
930ec58
Compare
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Supersedes #3349.
Closes #2862.
#3349 collected far too many commits due to not being able to replicate the CI Action behaviour locally. This alternative branch did a mass rebase, grouping the commits into a few basic categories and collapsing each category into a single commit, to make the history vastly cleaner.
There may be other checks for which it could be beneficial to introduce into the enforcement check, with corresponding rebasing of the repository (shouldn't preclude addition of a bad pattern if there's precedent for that pattern), but I think for now I'd prefer to get this completed and merged in so I can get on to dealing with the myriad conflicts.