Look at flipping clang-tidy's misc-* to enable-by-default #4699
Conversation
This file contains 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
|
@zygoloid Particularly interested in your recursion thoughts here. |
jonmeow
force-pushed
the
clang-tidy-misc
branch
from
b1e2ce8 to
117e79d
Compare
zygoloid
approved these changes
Dec 17, 2024
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
chandlerc
approved these changes
Dec 17, 2024
|
Note, per discussion, I'll be looking more at misc-non-private-member-variables-in-classes but in a separate PR. |
auto-merge was automatically disabled
December 17, 2024 21:36
Pull Request is not mergeable
github-merge-queue bot
pushed a commit
that referenced
this pull request
Dec 19, 2024
…to match (#4702) Pursuant to discussion regarding #4699, turn on `misc-non-private-member-variables-in-classes` using the `IgnoreClassesWithAllMemberVariablesBeingPublic` flag (the check treats structs as classes, so we need this for structs with all-public members). Updates the style guide notes to match, which should be pretty minor due to the scoping of test fixtures. Also fixes some underscore uses in test files on the way. Basically this is keeping the style for [class data member naming](https://google.github.io/styleguide/cppguide.html#Variable_Names) even while making them public.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
I was wondering, instead of treating
miscdifferently and enabling specific checks, maybe we can flip that since we actually seem okay with most of the checks?The main check I'm enabling, with significant edits here, is
misc-no-recursion. But maybe this is helpful to enable, even with the necessary NOLINTs, since we want to avoid recursion in the toolchain? This PR shows some example fixes in subst.cpp (which are more stylistic, since the code shouldn't actually have recursed due to its structure; I think we could remove the warning on TryResolveInst the same way). Some also just don't seem worth fixing, like those in tests files (I didn't see a way to exclude files in .clang-tidy, so instead I'm using NOLINTBEGIN). But I think we might actually want to fix inst_namer, and there's enough in convert that I didn't look closely.Also, I made some protected -> private style fixes based on
misc-non-private-member-variables-in-classes(this is also how I noticedclass Realversusstruct Real). With node_stack, it looks like theprotectedwasn't even used. Per style, data members should be private outside tests. But since we can't trivially excludeprotectedmembers in tests, I'm turning it off -- I don't view it as offering enough benefit on the whole.migrate_cpp issues are preexisting (I believe we just aren't monitoring it), but changes there make
bazel build --config=clang-tidy -k //...work cleanly.