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

dynamic_modules: exclude abi.h from clang-tidy #37317

Merged
merged 3 commits into from
Nov 25, 2024

Conversation

mathetake
Copy link
Member

@mathetake mathetake commented Nov 22, 2024

Commit Message: dynamic_modules: dynamic_modules: exclude abi.h from clang-tidy
Additional Description:

This commit disables clang-tidy for the whole ABI header which is a pure C file.
Previously, I added // NOLINT comment line on each place,
clang-tidy is only for C++ source code, so this disables it considering the
readability. This shouldn't be a problem as the header only has types and
function declarations.

Risk Level: none
Testing: n/a
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
Copy link

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #37317 was opened by mathetake.

see: more, trace.

@mathetake mathetake changed the title dynamic_modules: exclude ABI C header from '-modernize-use-using' dynamic_modules: exclude abi.h from '-modernize-use-using' Nov 22, 2024
@mathetake
Copy link
Member Author

cc @phlax

@mathetake mathetake marked this pull request as ready for review November 22, 2024 16:32
@mathetake
Copy link
Member Author

Note: I wanted to use // NOLINTBEGIN(modernize-use-using) rather than specifying in .clang-tidy.yaml but looks like it's available from LLVM 15 vs we are still using 14

@mathetake
Copy link
Member Author

so as discussed in #37320 clang-tidy is currently disabled in the CI, so the change here is not checked there... At least it's working locally ;)

.clang-tidy Outdated Show resolved Hide resolved
@phlax phlax self-assigned this Nov 24, 2024
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
@mathetake mathetake changed the title dynamic_modules: exclude abi.h from '-modernize-use-using' dynamic_modules: exclude abi.h from clang-tidy Nov 25, 2024
@mathetake
Copy link
Member Author

@phlax i went ahead and simply put the header in the ignore target of clang-tidy - see the description of the PR for detail!

Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
@mathetake mathetake requested a review from phlax November 25, 2024 16:30
Copy link
Member

@phlax phlax left a comment

Choose a reason for hiding this comment

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

lgtm, thanks @mathetake

@phlax phlax merged commit 44ff4a0 into envoyproxy:main Nov 25, 2024
24 checks passed
@mathetake mathetake deleted the clangtidy branch November 25, 2024 17:17
phlax pushed a commit that referenced this pull request Nov 26, 2024
…#37378)

This directive was introduced in #37317 but turns out this is not
available in LLVM 14.
So for now we remove this. By the time we enable clang-tidy on CI we
should find
a way to ignore it, but for now simply remove it so that we can run
clang-tidy locally.

part of #28566

Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
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