I was wondering, instead of treating `misc` differently 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
noticed `class Real` versus `struct Real`). With node_stack, it looks
like the `protected` wasn't even used. [Per
style](https://google.github.io/styleguide/cppguide.html#Access_Control),
data members should be private outside tests. But since we can't
trivially exclude `protected` members 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.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
This was failing to build for me locally with some arbitrary Clang HEAD
host compiler:
```
migrate_cpp/rewriter.cpp:225:3: error: call to member function 'SetReplacement' is ambiguous
225 | SetReplacement(expr, {OutputSegment(std::move(text))});
| ^~~~~~~~~~~~~~
./migrate_cpp/rewriter.h:141:8: note: candidate function [with T = clang::IntegerLiteral]
141 | auto SetReplacement(const T* node, std::vector<OutputSegment> output_segments)
| ^
./migrate_cpp/rewriter.h:150:8: note: candidate function [with T = clang::IntegerLiteral]
150 | auto SetReplacement(const T* node, OutputSegment segment) -> void {
| ^
```
No idea if that's a bug in clang HEAD, but it seemed like removing the
{} simplified the code anyway - so here's that.
This does some more work to the run_clang_tidy.py wrapper script, and runs an example pass.
"again" because it's really the proto fuzzer changes that broke it, it had been working before.
"mostly" because there's still an issue within the proto fuzzer that it can't find "port/protobuf.h", i.e. https://github.com/google/libprotobuf-mutator/tree/master/port, but I'm still hesitant to add an include path there.
The approach uses a RecursiveASTVisitor rather than matchers. Matchers
and callbacks do not compose neatly and introduce significant runtime overhead
over RecursiveASTVisitor when an action needs to be performed on most nodes.
Jon Ross-PerkinsandRichard Smith
David Blaikie
Andy Soffer