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

Option to allow acting against different files for clang-tidy and clang-format #233

Closed
mirenradia opened this issue Apr 22, 2024 · 4 comments · Fixed by #247
Closed

Option to allow acting against different files for clang-tidy and clang-format #233

mirenradia opened this issue Apr 22, 2024 · 4 comments · Fixed by #247
Labels
enhancement New feature or request

Comments

@mirenradia
Copy link
Contributor

mirenradia commented Apr 22, 2024

This is a small feature request to add the capability to act against different files for clang-tidy and clang-format in the same job.

Motivation

In the C++ project I am using this action against, we sometimes separate out the declaration of templates (e.g. a MyTemplateClass.hpp file) and implementations of these templates (e.g. MyTemplateClass.impl.hpp). The .impl.hpp files shouldn't be linted on their own as they only make sense when included directly in the template declaration file and they contain an include guard to that effect such as

#ifndef MYTEMPLATECLASS_HPP_
#error "This file should only be included through MyTemplateClass.hpp"
#endif

#ifndef MYTEMPLATECLASS_IMPL_HPP_
#define MYTEMPLATECLASS_IMPL_HPP_

<implementation code here>

#endif

This leads to cpp-linter reporting errors such as

/path/to/MyTemplateClass.impl.hpp:x:y: error: no template named 'MyTemplateClass' [clang-diagnostic-error]
   x | inline MyTemplateClass<T>::foo(
     |

which I'd like to avoid.

Other options I've considered to get around this:

  • Explicitly passing all .impl.hpp header files to the ignore argument: I would then miss all warnings from these files which I'd like to see.
  • Renaming the extensions of template implementation files to .ipp so that cpp-linter doesn't act on them (assuming I didn't add this to the extensions argument): this would mean their formatting is not checked either.
  • Running separate jobs for clang-format and clang-tidy and doing the above: I think this would work but it seems a little wasteful and I quite like the fact I can do both formatting and linting in 1 job with this action.

I'm open to other suggestions if there's already a way to get around the issue I describe that I haven't thought of.

@2bndy5
Copy link
Collaborator

2bndy5 commented Apr 22, 2024

Sounds like you want a tool-specific file filter, yes?

This seems reasonable. And I've been re-thinking about adding glob support in the ignore option. Using globs seems more feasible than using regular expression patterns (specified in yaml as a single-line string).

My first impression is to have ignore-tidy and ignore-format options that accept a value similar to ignore but are only applied according to the tool that is being invoked.

@2bndy5 2bndy5 added the enhancement New feature or request label Apr 22, 2024
@2bndy5
Copy link
Collaborator

2bndy5 commented Apr 22, 2024

Running separate jobs for clang-format and clang-tidy and doing the above: I think this would work but it seems a little wasteful and I quite like the fact I can do both formatting and linting in 1 job with this action.

Currently, this is the only way to achieve what you want done. I used to do this on a project where certain platforms followed different style guides. However, this is not the best idea for producing digestible output (thread-comments, tidy-review, etc.)

@mirenradia
Copy link
Contributor Author

Sounds like you want a tool-specific file filter, yes?

This seems reasonable. And I've been re-thinking about adding glob support in the ignore option. Using globs seems more feasible than using regular expression patterns (specified in yaml as a single-line string).

My first impression is to have ignore-tidy and ignore-format options that accept a value similar to ignore but are only applied according to the tool that is being invoked.

I don't want to ignore the files completely (like e.g. a header provided by an external library) as I want to see clang-tidy warnings that appear from them when they are included elsewhere but I don't want clang-tidy to act on them directly if that makes sense?

@2bndy5
Copy link
Collaborator

2bndy5 commented Apr 23, 2024

as I want to see clang-tidy warnings that appear from them when they are included elsewhere but I don't want clang-tidy to act on them directly if that makes sense?

This makes sense to me.

- uses: cpp-linter/cpp-linter-action@v2
  with:
    ignore: .github          # files ignored by both tidy and format
    ignore-tidy: '*.impl.h'  # files ignored by only clang-tidy
    ignore-format: '*.h.in'  # flies ignored by only clang-format

@2bndy5 2bndy5 linked a pull request May 6, 2024 that will close this issue
2bndy5 added a commit that referenced this issue Jun 7, 2024
@2bndy5 2bndy5 linked a pull request Jun 7, 2024 that will close this issue
2bndy5 added a commit that referenced this issue Jun 8, 2024
* Bump the pip group across 1 directory with 2 updates

Bumps the pip group with 2 updates in the / directory: [clang-tools](https://github.com/cpp-linter/clang-tools-pip) and [cpp-linter](https://github.com/cpp-linter/cpp-linter).


Updates `clang-tools` from 0.12.1 to 0.13.0
- [Release notes](https://github.com/cpp-linter/clang-tools-pip/releases)
- [Commits](cpp-linter/clang-tools-pip@v0.12.1...v0.13.0)

Updates `cpp-linter` from 1.8.1 to 1.10.0
- [Release notes](https://github.com/cpp-linter/cpp-linter/releases)
- [Commits](cpp-linter/cpp-linter@v1.8.1...v1.10.0)

---
updated-dependencies:
- dependency-name: clang-tools
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: pip
- dependency-name: cpp-linter
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: pip
...

Signed-off-by: dependabot[bot] <[email protected]>

* add tool-specific ignore options

resolves #233

* add input for passive reviews

resolves #243

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Brendan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants