Add Perl engine to check_syntax for line-numbered reporting - #3400
Draft
Lestropie wants to merge 3 commits into
Draft
Add Perl engine to check_syntax for line-numbered reporting#3400Lestropie wants to merge 3 commits into
Lestropie wants to merge 3 commits into
Conversation
Reworked the C++ house-style checker so violations are reported with their source line numbers. The original Bash pipeline progressively stripped and joined lines, discarding the positional information needed to point developers at offending code; this introduces a dedicated Perl engine that mirrors every transform while maintaining a character-to-source-line map, yielding exact lines or ranges even for multi-line matches. Diff mode now separates hunks structurally rather than via in-band sentinels, eliminating false positives that previously spanned non-contiguous blocks. The check_syntax front-end retains its whole-tree, staged-diff, and explicit file-list modes while delegating pattern matching to the engine, which is invoked through perl and so needs no execute bit. The in-house check patterns are consolidated into a single documented block, each annotated with the rule it enforces and the working buffer it scans. Session prompts: 1. > Propose modifications to script "check_syntax" to provide line numbers where syntax changes are required. Each file is progressively filtered, initially by removal of lines and later by removing newline characters, so different solutions for preserving line numbers through those checks to be capable of printing them in the output may be required. Generate multiple candidate proposals. Given some matches may span multiple lines, any manual insertion of line numbers into code must not interfere with grep matching. Include in proposals more radical alterations to achieve this; eg. switching languages, use of Abstract Syntax Tree. 2. > Prototype the Candidate 3 Perl engine 3. > 1. Investigate whether the current solution for executing check_syntax on diff mode hunks, explicitly precluding the merging of blocks of code onto a single line to preclude false positives that span across such blocks, has a more elegant solution in this Perl re-write. > 2. Add Perl code to repository under testing/tools, and modify check_syntax script to provide existing command-line interface capabilities but invoke the Perl code to do the heavy-lifting. > 3. Generate a dummy source code file in testing/data/ that contains, for each check in place, a combination of offending code (using different combinations for each check if relevant, eg. presence/absence of whitespace, different true positives checked for), and hypothetical false positives if regex designs are not diligent enough (eg. matching part of regex but in wrong surrounding context). Then add a unit test ensuring that the set of detected offending lines matches prior expectation. 4. > Move check_syntax_engine.pl to repository root alongside check_syntax. If it is possible to do so, remove execution permissions from check_syntax_engine.pl, executing from check_syntax by explicitly invoking perl. 5. > 1. In script check_syntax, restore the capability to provide as command-line input a list of files to be analysed. > 2. In check_syntax_engine.pl, move the set of in-house regular expressions to a code block after the argument parsing, commenting for each the check being performed and the string buffer to which it is applied. Generated-by: Claude Opus 4.8 <noreply@anthropic.com>
![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.
Member
|
Why Perl instead of Python? I feel like the latter is a far more suitable choice for MRtrix3 since it won't require an additional dependency. |
Member
Author
Went with Claude's 'instinct'. Existing checks are already PCRE, and Perl is present on all but the most aggressively pruned distributions. Presence of variable-length look-behinds means that Python It also wouldn't surprise me terribly if it turned out to execute faster than Python. Having said that it's pretty easy to just ask Claude for a Python version and see what happens for the sake of language consistency / maintainability. |
Member
Author
Whole-repo analysis: 4.6s Perl, 39s Python. |
Exclude testing/data/ directory from check_syntax and run_pylint to avoid flagging intentionally non-conforming fixtures. Fix update_copyright to preserve separator lines (blank lines and bare comment characters) between shebangs and copyright notices, which were previously erased. Set up unittest_check_syntax_engine to run against build-directory copies of the Perl engine and fixture file via CMake configure_file and environment variables, with fallbacks for manual test execution. Prompt: > 1. For syntax checking scripts, exclude all contents of directory testing/data/. > 2. Script "update_copyright" erroneously removes the "#" comment character from the second line of file "check_syntax_engine.pl". Diagnose and fix. > 3. Unit test "unittest_check_syntax_engine" requires that files "check_syntax_engine.pl" and "testing/data/check_syntax/offenders.cpp" have been placed inside the build directory in order to be accessible by ctest. Report on the actual locations of these files utilised during this test. If the source code repository is used at all, ensure that these files are instead suitably copied into the build directory when MRTRIX_BUILD_TESTS=ON. Generated-by: Claude Opus 4.8 <noreply@anthropic.com>
Update clang-tidy-enforce and clang-tidy-review CI workflows to exclude the testing/data directory from style linting. This directory contains intentionally non-conforming code samples and fixtures that should not be subject to clang-tidy checks. Excluding this directory prevents spurious style violations from these fixtures appearing in CI. Prompt: > Additionally ensure that the clang-tidy-review and clang-tidy-enforce CI checks do not process the contents of testing/data/. Generated-by: Claude Opus 4.8 <noreply@anthropic.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
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.
This ports the heavy-lifting portion of the
check_syntaxscript from a Bash script with repeatedgrepcalls per file to a Perl executable. The initial motivation was providing offending line numbers, to expedite instructing Claude on how to correct its own violations. As a happy side-effect, run-time when executing over the whole code base drops from a few minutes to a few seconds.Check whether the explicit unit test for
check_syntaxneeds to be removed. Neithercheck_syntaxnorcheck_syntax_engine.plare present in the build directory. It might be simpler to not have such a test, and just have an internal expectation that any modifications tocheck_syntax*will come with corresponding changes totesting/diff/check_syntax/offenders.cppand will be run manually to confirm operation.Consider filesystem location. This is currently an additional file at repository root. When I get to Changes to testing file structure #3083 I'd like to either move both of these into
cpp/, or create a new root-level directory containing production scripts likecheck_syntax,generate_user_docs.sh,run_pylint, ...