Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixed #12017/#13723 - added command-line option --clang-tidy{=<exe>} and fixed output #7366

Merged
merged 3 commits into from
Apr 7, 2025

Conversation

firewave
Copy link
Collaborator

@firewave firewave commented Mar 9, 2025

No description provided.

@firewave
Copy link
Collaborator Author

firewave commented Mar 9, 2025

Still needs a Python test and requires #7285 and #7291 to be merged.

Prerequisite for fixing https://trac.cppcheck.net/ticket/12053.

@firewave firewave force-pushed the clang-tidy-cli branch 2 times, most recently from 7b1e270 to 717b621 Compare March 18, 2025 20:29
@firewave firewave changed the title fixed #12017 - added command-line option --clang-tidy{=<exe>} fixed #12017/#13723 - added command-line option --clang-tidy{=<exe>} and fixed output Mar 18, 2025
@firewave
Copy link
Collaborator Author

Need to make sure it is run within the CI (i.e. clang-tidy is available).

@firewave
Copy link
Collaborator Author

Need to make sure it is run within the CI (i.e. clang-tidy is available).

Confirmed.

test/cli/other_test.py::test_clang_tidy_project 
[gw0] [ 71%] PASSED test/cli/other_test.py::test_clang_tidy_project

@firewave firewave marked this pull request as ready for review March 19, 2025 08:12
// TODO: is this even necessary?
// append .exe if it is not a path
if (Path::fromNativeSeparators(mSettings.clangTidyExecutable).find('/') == std::string::npos) {
exe += ".exe";
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if exe ends with ".exe" then don't append ".exe".

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but.. I agree with the TODO comment. Use the clangTidyExecutable as-is. Use the exact path provided by the user.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will extend the tests when I fix the missing invocations.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this code should just use clangTidyExecutable as-is. Remove the whole #ifdef ..

     const std::string exe = mSettings.clangTidyExecutable;

If the user writes --clang-tidy=clang-tidy.exe then you will add the ".exe" that feels bad.

If you really want to append the ".exe" then that should be done in the initialization in settings.h:

    /** Custom clang-tidy executable */
#ifdef _WIN32
    std::string clangTidyExecutable = "clang-tidy.exe";
#else
    std::string clangTidyExecutable = "clang-tidy";
#endif

As you said maybe this is not necessary anyway.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I said I will address this when the invocation are fixed in a follow-up since it is out-of-scope of the changes here.

The tests for this are also currently missing and I would have to add several overrides because invoking it is partially broken.

@firewave
Copy link
Collaborator Author

firewave commented Apr 5, 2025

Anything still to do here?

Copy link
Owner

@danmar danmar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alright then.. if you do continue and look at the invocations asap after this then feel free to merge this now..

@firewave firewave merged commit 2cea0d0 into danmar:main Apr 7, 2025
60 checks passed
@firewave firewave deleted the clang-tidy-cli branch April 7, 2025 09:22
@firewave
Copy link
Collaborator Author

firewave commented Apr 7, 2025

alright then.. if you do continue and look at the invocations asap after this then feel free to merge this now..

I just had another look and it is unfortunately not straight forward to fix.

But on the upside it should automatically fix itself when FileWithDetails and FileSettings have finally been merged. But that requires the language and the define (which will also involve simplecpp changes) handling to be fixed first. So it will probably be a while given the scope and there are still more pressing core issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants