Further expansions of check_syntax#3313
Open
Lestropie wants to merge 8 commits into
Open
Conversation
check_syntax has been used to find code where the size of a container has been compared to zero; for containers that do not themselves offer a .empty() function, use of a std::optional better communicates the prospect that data may be present or absent.
Conflicts: cpp/cmd/mrregister.cpp cpp/cmd/sh2amp.cpp cpp/cmd/tcksample.cpp cpp/cmd/transformcompose.cpp cpp/core/dwi/tractography/ACT/shared.h cpp/core/dwi/tractography/algorithms/sd_stream.h cpp/core/dwi/tractography/seeding/list.h cpp/core/dwi/tractography/seeding/seeding.cpp
Prompt: "Modify parsing of command-line usage within script "check_syntax". If one or more filesystem paths are provided, then syntax checks should be performed only for the content within those specific files. Abort if file does not exist. Abort if file does not have .h or .cpp extension. Abort if both --diff and at least one path are specified.". Generated-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Member
Author
|
@jdtournier Feel like it needs your approval to change the return type of some very long-standing API functions. See eg. |
Member
Author
|
Worth noting that this PR expects use of |
This was referenced May 7, 2026
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
1 participant
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.
Having discovered
std::optional, I now really dislike checks for the number of rows in a matrix being zero as a flag for the absence of data. I findstd::optionalfar more communicative in this respect. And it also has the prospect of distinguishing between something that was never present in the first place, versus something that was present but has been whittled away to zero size.std::unique_ptr<>for eg. aProgressBarthat is optionally constructed.std::optionalof a matrix very clearly communicates whether that function must provide relevant data, or whether it may return nothing if no such data are available.Expanding the regex searches to include
newanddeleteis trivial, and it would make sense to me to maximise use ofstd::make_unique<>andstd::make_shared<>and minimise use of naked pointers. There's a couple of places incpp/core/where there's still naked pointers in use---recalling particularlyThread::Queueand global tractography---but mostly we can get rid of them.The GUI code is however harder. There's a lot of passing responsibility for deleting dynamically-allocated objects to Qt, and lists of pointers etc.. So for now I've just excluded
cpp/guifrom these checks. Will have to see how difficult it is to resolve this against Run check_syntax as pre-commit hook #3310...Edit: This turned out to require no intervention:
run_checks()had already been set up to pass the filename regardless of whether looping over all content of each file or parsing agit diff, which occurs one file at a time.