Finish making clang-tidy (mostly) work (again) and run -fix (carbon-l…

…anguage#2312)

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.

common/indirect_value_test.cpp

@@ -58,7 +58,7 @@ struct TestValue {
  TestValue(TestValue&& other) noexcept : state("move constructed") {
  other.state = "move constructed from";
  }
- auto operator=(const TestValue&) noexcept -> TestValue& {
+ auto operator=(const TestValue& /*unused*/) noexcept -> TestValue& {
  state = "copy assigned";
  return *this;
  }

common/string_helpers_test.cpp

@@ -11,7 +11,6 @@
 
 #include "llvm/Support/Error.h"
 
-using ::llvm::toString;
 using ::testing::Eq;
 using ::testing::Optional;
 

compile_flags.txt

@@ -98,6 +98,14 @@ bazel-bin/external/com_google_googletest/googletest
 bazel-execroot/external/com_google_googletest/googletest/include
 -isystem
 bazel-bin/external/com_google_googletest/googletest/include
+-isystem
+bazel-execroot/external/com_google_libprotobuf_mutator/src
+-isystem
+bazel-bin/external/com_google_libprotobuf_mutator/src
+-isystem
+bazel-execroot/external/com_github_protocolbuffers_protobuf/src
+-isystem
+bazel-bin/external/com_github_protocolbuffers_protobuf/src
 -std=c++17
 -stdlib=libc++
 -no-canonical-prefixes

explorer/ast/bindings.h

@@ -6,6 +6,7 @@
 #define CARBON_EXPLORER_AST_BINDINGS_H_
 
 #include <map>
+#include <utility>
 
 #include "explorer/common/nonnull.h"
 #include "llvm/ADT/ArrayRef.h"
@@ -40,18 +41,19 @@ class Bindings {
  -> Nonnull<const Bindings*>;
 
  // Create an empty set of bindings.
- Bindings() {}
+ Bindings() = default;
 
  // Create an instantiated set of bindings for use during evaluation,
  // containing both arguments and witnesses.
  Bindings(BindingMap args, ImplWitnessMap witnesses)
- : args_(args), witnesses_(witnesses) {}
+ : args_(std::move(args)), witnesses_(std::move(witnesses)) {}
 
  enum NoWitnessesTag { NoWitnesses };
 
  // Create a set of bindings for use during type-checking, containing only the
  // arguments but not the corresponding witnesses.
- Bindings(BindingMap args, NoWitnessesTag) : args_(args), witnesses_() {}
+ Bindings(BindingMap args, NoWitnessesTag /*unused*/)
+ : args_(std::move(args)) {}
 
  // Add a value, and perhaps a witness, for a generic binding.
  void Add(Nonnull<const GenericBinding*> binding, Nonnull<const Value*> value,

explorer/ast/declaration.h

@@ -316,7 +316,7 @@ class MixinDeclaration : public Declaration {
  std::vector<Nonnull<Declaration*>> members)
  : Declaration(AstNodeKind::MixinDeclaration, source_loc),
  name_(std::move(name)),
- params_(std::move(params)),
+ params_(params),
  self_(self),
  members_(std::move(members)) {}
 
@@ -489,7 +489,7 @@ class InterfaceDeclaration : public Declaration {
  std::vector<Nonnull<Declaration*>> members)
  : Declaration(AstNodeKind::InterfaceDeclaration, source_loc),
  name_(std::move(name)),
- params_(std::move(params)),
+ params_(params),
  self_type_(arena->New<SelfDeclaration>(source_loc)),
  members_(std::move(members)) {
  // `interface X` has `Self:! X`.
@@ -688,10 +688,10 @@ class AliasDeclaration : public Declaration {
  public:
  using ImplementsCarbonValueNode = void;
 
- explicit AliasDeclaration(SourceLocation source_loc, const std::string& name,
+ explicit AliasDeclaration(SourceLocation source_loc, std::string name,
  Nonnull<Expression*> target)
  : Declaration(AstNodeKind::AliasDeclaration, source_loc),
- name_(name),
+ name_(std::move(name)),
  target_(target) {}
 
  static auto classof(const AstNode* node) -> bool {

explorer/ast/expression.h

@@ -8,6 +8,7 @@
 #include <map>
 #include <optional>
 #include <string>
+#include <utility>
 #include <variant>
 #include <vector>
 
@@ -876,7 +877,7 @@ class RewriteWhereClause : public WhereClause {
  std::string member_name,
  Nonnull<Expression*> replacement)
  : WhereClause(WhereClauseKind::RewriteWhereClause, source_loc),
- member_name_(member_name),
+ member_name_(std::move(member_name)),
  replacement_(replacement) {}
 
  static auto classof(const AstNode* node) {

explorer/ast/impl_binding.h

@@ -80,10 +80,11 @@ class ImplBinding : public AstNode {
 
  // Return the original impl binding.
  auto original() const -> Nonnull<const ImplBinding*> {
- if (original_.has_value())
+ if (original_.has_value()) {
  return *original_;
- else
+ } else {
  return this;
+ }
  }
 
  // Set the original impl binding.

explorer/ast/member.cpp

@@ -15,23 +15,23 @@ Member::Member(Nonnull<const NamedValue*> struct_member)
  : member_(struct_member) {}
 
 auto Member::name() const -> std::string_view {
- if (const Declaration* decl = member_.dyn_cast<const Declaration*>()) {
+ if (const auto* decl = member_.dyn_cast<const Declaration*>()) {
  return GetName(*decl).value();
  } else {
  return member_.get<const NamedValue*>()->name;
  }
 }
 
 auto Member::type() const -> const Value& {
- if (const Declaration* decl = member_.dyn_cast<const Declaration*>()) {
+ if (const auto* decl = member_.dyn_cast<const Declaration*>()) {
  return decl->static_type();
  } else {
  return *member_.get<const NamedValue*>()->value;
  }
 }
 
 auto Member::declaration() const -> std::optional<Nonnull<const Declaration*>> {
- if (const Declaration* decl = member_.dyn_cast<const Declaration*>()) {
+ if (const auto* decl = member_.dyn_cast<const Declaration*>()) {
  return decl;
  }
  return std::nullopt;

explorer/ast/pattern.h

@@ -275,10 +275,11 @@ class GenericBinding : public Pattern {
 
  // Return the original generic binding.
  auto original() const -> Nonnull<const GenericBinding*> {
- if (original_.has_value())
+ if (original_.has_value()) {
  return *original_;
- else
+ } else {
  return this;
+ }
  }
  // Set the original generic binding.
  void set_original(Nonnull<const GenericBinding*> orig) { original_ = orig; }

explorer/fuzzing/ast_to_proto.cpp

@@ -772,7 +772,7 @@ static auto DeclarationToProto(const Declaration& declaration)
  return declaration_proto;
 }
 
-Fuzzing::CompilationUnit AstToProto(const AST& ast) {
+auto AstToProto(const AST& ast) -> Fuzzing::CompilationUnit {
  Fuzzing::CompilationUnit compilation_unit;
  *compilation_unit.mutable_package_statement() =
  LibraryNameToProto(ast.package);

explorer/fuzzing/ast_to_proto_test.cpp

@@ -121,7 +121,7 @@ TEST(AstToProtoTest, SetsAllProtoFields) {
 } // namespace
 } // namespace Carbon::Testing
 
-int main(int argc, char** argv) {
+auto main(int argc, char** argv) -> int {
  ::testing::InitGoogleTest(&argc, argv);
  // gtest should remove flags, leaving just input files.
  Carbon::Testing::carbon_files =

explorer/fuzzing/explorer_fuzzer.cpp

@@ -2,7 +2,7 @@
 // Exceptions. See /LICENSE for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 
-#include <libprotobuf_mutator/src/libfuzzer/libfuzzer_macro.h>
+#include <libfuzzer/libfuzzer_macro.h>
 
 #include "common/error.h"
 #include "explorer/fuzzing/fuzzer_util.h"

explorer/interpreter/action_stack.h

@@ -90,7 +90,7 @@ class ActionStack {
  -> ErrorOr<Success>;
  // Replace the current action with another action that produces the same kind
  // of result and run it next.
- auto ReplaceWith(std::unique_ptr<Action> child) -> ErrorOr<Success>;
+ auto ReplaceWith(std::unique_ptr<Action> replacement) -> ErrorOr<Success>;
 
  // Start a new recursive action.
  auto BeginRecursiveAction() {

explorer/interpreter/builtins.h

@@ -20,7 +20,7 @@ namespace Carbon {
 
 class Builtins {
  public:
- explicit Builtins() {}
+ explicit Builtins() = default;
 
  enum class Builtin {
  // Conversions.

explorer/interpreter/impl_scope.cpp

@@ -11,7 +11,6 @@
 
 using llvm::cast;
 using llvm::dyn_cast;
-using llvm::isa;
 
 namespace Carbon {
 

explorer/interpreter/impl_scope.h

@@ -158,7 +158,7 @@ class ImplScope {
 // scope.
 struct SingleStepEqualityContext : public EqualityContext {
  public:
- SingleStepEqualityContext(Nonnull<const ImplScope*> impl_scope)
+ explicit SingleStepEqualityContext(Nonnull<const ImplScope*> impl_scope)
  : impl_scope_(impl_scope) {}
 
  // Visits the values that are equal to the given value and a single step away

explorer/interpreter/interpreter.cpp

@@ -160,7 +160,7 @@ class Interpreter {
 
  void PrintState(llvm::raw_ostream& out);
 
- Phase phase() const { return phase_; }
+ auto phase() const -> Phase { return phase_; }
 
  Nonnull<Arena*> arena_;
 
@@ -830,7 +830,7 @@ auto Interpreter::CallFunction(const CallExpression& call,
  alt.alt_name(), alt.choice_name(), arg));
  }
  case Value::Kind::FunctionValue: {
- const FunctionValue& fun_val = cast<FunctionValue>(*fun);
+ const auto& fun_val = cast<FunctionValue>(*fun);
  const FunctionDeclaration& function = fun_val.declaration();
  if (!function.body().has_value()) {
  return ProgramError(call.source_loc())
@@ -1213,7 +1213,7 @@ auto Interpreter::StepExp() -> ErrorOr<Success> {
  }
  }
  case ExpressionKind::CallExpression: {
- const CallExpression& call = cast<CallExpression>(exp);
+ const auto& call = cast<CallExpression>(exp);
  unsigned int num_impls = call.impls().size();
  if (act.pos() == 0) {
  // { {e1(e2) :: C, E, F} :: S, H}
@@ -1225,12 +1225,12 @@ auto Interpreter::StepExp() -> ErrorOr<Success> {
  // -> { { e :: v([]) :: C, E, F} :: S, H}
  return todo_.Spawn(
  std::make_unique<ExpressionAction>(&call.argument()));
- } else if (num_impls > 0 && act.pos() < 2 + int(num_impls)) {
+ } else if (num_impls > 0 && act.pos() < 2 + static_cast<int>(num_impls)) {
  auto iter = call.impls().begin();
  std::advance(iter, act.pos() - 2);
  return todo_.Spawn(
  std::make_unique<WitnessAction>(cast<Witness>(iter->second)));
- } else if (act.pos() == 2 + int(num_impls)) {
+ } else if (act.pos() == 2 + static_cast<int>(num_impls)) {
  // { { v2 :: v1([]) :: C, E, F} :: S, H}
  // -> { {C',E',F'} :: {C, E, F} :: S, H}
  ImplWitnessMap witnesses;
@@ -1243,12 +1243,13 @@ auto Interpreter::StepExp() -> ErrorOr<Success> {
  }
  return CallFunction(call, act.results()[0], act.results()[1],
  std::move(witnesses));
- } else if (act.pos() == 3 + int(num_impls)) {
+ } else if (act.pos() == 3 + static_cast<int>(num_impls)) {
  if (act.results().size() < 3 + num_impls) {
  // Control fell through without explicit return.
  return todo_.FinishAction(TupleValue::Empty());
  } else {
- return todo_.FinishAction(act.results()[2 + int(num_impls)]);
+ return todo_.FinishAction(
+ act.results()[2 + static_cast<int>(num_impls)]);
  }
  } else {
  CARBON_FATAL() << "in StepExp with Call pos " << act.pos();
@@ -1292,7 +1293,8 @@ auto Interpreter::StepExp() -> ErrorOr<Success> {
  CARBON_ASSIGN_OR_RETURN(
  Nonnull<const Value*> string_value,
  Convert(args[1], arena_->New<StringType>(), exp.source_loc()));
- if (cast<BoolValue>(condition)->value() == false) {
+ bool condition_value = cast<BoolValue>(condition)->value();
+ if (!condition_value) {
  return ProgramError(exp.source_loc()) << *string_value;
  }
  return todo_.FinishAction(TupleValue::Empty());
@@ -1679,8 +1681,8 @@ auto Interpreter::StepStmt() -> ErrorOr<Success> {
  std::make_unique<ExpressionAction>(&cast<For>(stmt).loop_target()));
  }
  if (act.pos() == 1) {
- Nonnull<const TupleValue*> source_array =
- cast<const TupleValue>(act.results()[TargetVarPosInResult]);
+ const auto* source_array =
+ cast<TupleValue>(act.results()[TargetVarPosInResult]);
 
  auto end_index = static_cast<int>(source_array->elements().size());
  if (end_index == 0) {
@@ -1692,11 +1694,10 @@ auto Interpreter::StepStmt() -> ErrorOr<Success> {
  &cast<For>(stmt).variable_declaration()));
  }
  if (act.pos() == 2) {
- Nonnull<const BindingPlaceholderValue*> loop_var =
- cast<const BindingPlaceholderValue>(
- act.results()[LoopVarPosInResult]);
- Nonnull<const TupleValue*> source_array =
- cast<const TupleValue>(act.results()[TargetVarPosInResult]);
+ const auto* loop_var =
+ cast<BindingPlaceholderValue>(act.results()[LoopVarPosInResult]);
+ const auto* source_array =
+ cast<TupleValue>(act.results()[TargetVarPosInResult]);
 
  auto start_index =
  cast<IntValue>(act.results()[CurrentIndexPosInResult])->value();
@@ -1714,11 +1715,10 @@ auto Interpreter::StepStmt() -> ErrorOr<Success> {
  cast<IntValue>(act.results()[EndIndexPosInResult])->value();
 
  if (current_index < end_index) {
- Nonnull<const TupleValue*> source_array =
+ const auto* source_array =
  cast<const TupleValue>(act.results()[TargetVarPosInResult]);
- Nonnull<const BindingPlaceholderValue*> loop_var =
- cast<const BindingPlaceholderValue>(
- act.results()[LoopVarPosInResult]);
+ const auto* loop_var = cast<const BindingPlaceholderValue>(
+ act.results()[LoopVarPosInResult]);
 
  CARBON_ASSIGN_OR_RETURN(
  Nonnull<const Value*> assigned_array_element,
@@ -2003,7 +2003,7 @@ auto Interpreter::StepDeclaration() -> ErrorOr<Success> {
 
 auto Interpreter::StepCleanUp() -> ErrorOr<Success> {
  Action& act = todo_.CurrentAction();
- CleanupAction& cleanup = cast<CleanupAction>(act);
+ auto& cleanup = cast<CleanupAction>(act);
  if (act.pos() < cleanup.locals_count()) {
  auto lvalue = act.scope()->locals()[cleanup.locals_count() - act.pos() - 1];
  SourceLocation source_loc("destructor", 1);

explorer/interpreter/pattern_analysis.h

@@ -65,7 +65,7 @@ class AbstractPattern {
  private:
  // This is aligned so that we can use it in the `PointerUnion` below.
  struct alignas(8) WildcardTag {};
- AbstractPattern(WildcardTag)
+ explicit AbstractPattern(WildcardTag /*unused*/)
  : value_(static_cast<const WildcardTag*>(nullptr)), type_(nullptr) {}
 
  void Set(Nonnull<const Pattern*> pattern);