From 4d522c8e90b84fdfb371015a3b89eafe864bb067 Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Tue, 18 Oct 2022 15:48:17 -0700 Subject: [PATCH] Finish making clang-tidy (mostly) work (again) and run -fix (#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 | 2 +- common/string_helpers_test.cpp | 1 - compile_flags.txt | 8 ++++ explorer/ast/bindings.h | 8 ++-- explorer/ast/declaration.h | 8 ++-- explorer/ast/expression.h | 3 +- explorer/ast/impl_binding.h | 5 ++- explorer/ast/member.cpp | 6 +-- explorer/ast/pattern.h | 5 ++- explorer/fuzzing/ast_to_proto.cpp | 2 +- explorer/fuzzing/ast_to_proto_test.cpp | 2 +- explorer/fuzzing/explorer_fuzzer.cpp | 2 +- explorer/interpreter/action_stack.h | 2 +- explorer/interpreter/builtins.h | 2 +- explorer/interpreter/impl_scope.cpp | 1 - explorer/interpreter/impl_scope.h | 2 +- explorer/interpreter/interpreter.cpp | 40 ++++++++--------- explorer/interpreter/pattern_analysis.h | 2 +- explorer/interpreter/type_checker.cpp | 52 +++++++++++------------ explorer/interpreter/type_checker.h | 6 +-- explorer/interpreter/value.cpp | 17 ++++---- explorer/interpreter/value.h | 13 +++--- migrate_cpp/output_segment.h | 4 +- migrate_cpp/rewriter.cpp | 10 ++--- migrate_cpp/rewriter.h | 29 +++++++------ migrate_cpp/rewriter_test.cpp | 8 ++-- scripts/create_compdb.py | 18 ++++---- scripts/run_clang_tidy.py | 51 +++++++++++++++------- third_party/libprotobuf_mutator/BUILD.txt | 4 +- toolchain/lexer/token_kind_test.cpp | 2 +- toolchain/semantics/node_ref.h | 2 +- 31 files changed, 176 insertions(+), 141 deletions(-) diff --git a/common/indirect_value_test.cpp b/common/indirect_value_test.cpp index 345984d80867..62750ef43c47 100644 --- a/common/indirect_value_test.cpp +++ b/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; } diff --git a/common/string_helpers_test.cpp b/common/string_helpers_test.cpp index 1854035936c8..bcb715e3de23 100644 --- a/common/string_helpers_test.cpp +++ b/common/string_helpers_test.cpp @@ -11,7 +11,6 @@ #include "llvm/Support/Error.h" -using ::llvm::toString; using ::testing::Eq; using ::testing::Optional; diff --git a/compile_flags.txt b/compile_flags.txt index d66a3399bae6..38cb1f74ee98 100644 --- a/compile_flags.txt +++ b/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 diff --git a/explorer/ast/bindings.h b/explorer/ast/bindings.h index 61f72be46414..392719a24b34 100644 --- a/explorer/ast/bindings.h +++ b/explorer/ast/bindings.h @@ -6,6 +6,7 @@ #define CARBON_EXPLORER_AST_BINDINGS_H_ #include +#include #include "explorer/common/nonnull.h" #include "llvm/ADT/ArrayRef.h" @@ -40,18 +41,19 @@ class Bindings { -> Nonnull; // 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 binding, Nonnull value, diff --git a/explorer/ast/declaration.h b/explorer/ast/declaration.h index b53d1e8f9e0f..e9d55d4d0a94 100644 --- a/explorer/ast/declaration.h +++ b/explorer/ast/declaration.h @@ -316,7 +316,7 @@ class MixinDeclaration : public Declaration { std::vector> 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> members) : Declaration(AstNodeKind::InterfaceDeclaration, source_loc), name_(std::move(name)), - params_(std::move(params)), + params_(params), self_type_(arena->New(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 target) : Declaration(AstNodeKind::AliasDeclaration, source_loc), - name_(name), + name_(std::move(name)), target_(target) {} static auto classof(const AstNode* node) -> bool { diff --git a/explorer/ast/expression.h b/explorer/ast/expression.h index 58706481bb4a..1274839c85e5 100644 --- a/explorer/ast/expression.h +++ b/explorer/ast/expression.h @@ -8,6 +8,7 @@ #include #include #include +#include #include #include @@ -876,7 +877,7 @@ class RewriteWhereClause : public WhereClause { std::string member_name, Nonnull replacement) : WhereClause(WhereClauseKind::RewriteWhereClause, source_loc), - member_name_(member_name), + member_name_(std::move(member_name)), replacement_(replacement) {} static auto classof(const AstNode* node) { diff --git a/explorer/ast/impl_binding.h b/explorer/ast/impl_binding.h index 5c0db67515d4..59cca394a3e3 100644 --- a/explorer/ast/impl_binding.h +++ b/explorer/ast/impl_binding.h @@ -80,10 +80,11 @@ class ImplBinding : public AstNode { // Return the original impl binding. auto original() const -> Nonnull { - if (original_.has_value()) + if (original_.has_value()) { return *original_; - else + } else { return this; + } } // Set the original impl binding. diff --git a/explorer/ast/member.cpp b/explorer/ast/member.cpp index c6b4ef413ec0..965671ca5dd4 100644 --- a/explorer/ast/member.cpp +++ b/explorer/ast/member.cpp @@ -15,7 +15,7 @@ Member::Member(Nonnull struct_member) : member_(struct_member) {} auto Member::name() const -> std::string_view { - if (const Declaration* decl = member_.dyn_cast()) { + if (const auto* decl = member_.dyn_cast()) { return GetName(*decl).value(); } else { return member_.get()->name; @@ -23,7 +23,7 @@ auto Member::name() const -> std::string_view { } auto Member::type() const -> const Value& { - if (const Declaration* decl = member_.dyn_cast()) { + if (const auto* decl = member_.dyn_cast()) { return decl->static_type(); } else { return *member_.get()->value; @@ -31,7 +31,7 @@ auto Member::type() const -> const Value& { } auto Member::declaration() const -> std::optional> { - if (const Declaration* decl = member_.dyn_cast()) { + if (const auto* decl = member_.dyn_cast()) { return decl; } return std::nullopt; diff --git a/explorer/ast/pattern.h b/explorer/ast/pattern.h index 5d8d73025070..4ceeb100bd3a 100644 --- a/explorer/ast/pattern.h +++ b/explorer/ast/pattern.h @@ -275,10 +275,11 @@ class GenericBinding : public Pattern { // Return the original generic binding. auto original() const -> Nonnull { - if (original_.has_value()) + if (original_.has_value()) { return *original_; - else + } else { return this; + } } // Set the original generic binding. void set_original(Nonnull orig) { original_ = orig; } diff --git a/explorer/fuzzing/ast_to_proto.cpp b/explorer/fuzzing/ast_to_proto.cpp index 9a1e311063d4..5f2057658056 100644 --- a/explorer/fuzzing/ast_to_proto.cpp +++ b/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); diff --git a/explorer/fuzzing/ast_to_proto_test.cpp b/explorer/fuzzing/ast_to_proto_test.cpp index e3bda2fe8589..9e58e5e388da 100644 --- a/explorer/fuzzing/ast_to_proto_test.cpp +++ b/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 = diff --git a/explorer/fuzzing/explorer_fuzzer.cpp b/explorer/fuzzing/explorer_fuzzer.cpp index 406849990199..0e3d2184e9b3 100644 --- a/explorer/fuzzing/explorer_fuzzer.cpp +++ b/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 +#include #include "common/error.h" #include "explorer/fuzzing/fuzzer_util.h" diff --git a/explorer/interpreter/action_stack.h b/explorer/interpreter/action_stack.h index fb13e59c3753..4eccc95511f5 100644 --- a/explorer/interpreter/action_stack.h +++ b/explorer/interpreter/action_stack.h @@ -90,7 +90,7 @@ class ActionStack { -> ErrorOr; // Replace the current action with another action that produces the same kind // of result and run it next. - auto ReplaceWith(std::unique_ptr child) -> ErrorOr; + auto ReplaceWith(std::unique_ptr replacement) -> ErrorOr; // Start a new recursive action. auto BeginRecursiveAction() { diff --git a/explorer/interpreter/builtins.h b/explorer/interpreter/builtins.h index db38fa70e4b4..04d3a5c42bc2 100644 --- a/explorer/interpreter/builtins.h +++ b/explorer/interpreter/builtins.h @@ -20,7 +20,7 @@ namespace Carbon { class Builtins { public: - explicit Builtins() {} + explicit Builtins() = default; enum class Builtin { // Conversions. diff --git a/explorer/interpreter/impl_scope.cpp b/explorer/interpreter/impl_scope.cpp index e76407af954f..d06d759c7268 100644 --- a/explorer/interpreter/impl_scope.cpp +++ b/explorer/interpreter/impl_scope.cpp @@ -11,7 +11,6 @@ using llvm::cast; using llvm::dyn_cast; -using llvm::isa; namespace Carbon { diff --git a/explorer/interpreter/impl_scope.h b/explorer/interpreter/impl_scope.h index 0ddef15eef29..5abadc3fcb03 100644 --- a/explorer/interpreter/impl_scope.h +++ b/explorer/interpreter/impl_scope.h @@ -158,7 +158,7 @@ class ImplScope { // scope. struct SingleStepEqualityContext : public EqualityContext { public: - SingleStepEqualityContext(Nonnull impl_scope) + explicit SingleStepEqualityContext(Nonnull impl_scope) : impl_scope_(impl_scope) {} // Visits the values that are equal to the given value and a single step away diff --git a/explorer/interpreter/interpreter.cpp b/explorer/interpreter/interpreter.cpp index 721869535eef..22aac67b8311 100644 --- a/explorer/interpreter/interpreter.cpp +++ b/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_; @@ -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(*fun); + const auto& fun_val = cast(*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 { } } case ExpressionKind::CallExpression: { - const CallExpression& call = cast(exp); + const auto& call = cast(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 { // -> { { e :: v([]) :: C, E, F} :: S, H} return todo_.Spawn( std::make_unique(&call.argument())); - } else if (num_impls > 0 && act.pos() < 2 + int(num_impls)) { + } else if (num_impls > 0 && act.pos() < 2 + static_cast(num_impls)) { auto iter = call.impls().begin(); std::advance(iter, act.pos() - 2); return todo_.Spawn( std::make_unique(cast(iter->second))); - } else if (act.pos() == 2 + int(num_impls)) { + } else if (act.pos() == 2 + static_cast(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 { } 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(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(num_impls)]); } } else { CARBON_FATAL() << "in StepExp with Call pos " << act.pos(); @@ -1292,7 +1293,8 @@ auto Interpreter::StepExp() -> ErrorOr { CARBON_ASSIGN_OR_RETURN( Nonnull string_value, Convert(args[1], arena_->New(), exp.source_loc())); - if (cast(condition)->value() == false) { + bool condition_value = cast(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 { std::make_unique(&cast(stmt).loop_target())); } if (act.pos() == 1) { - Nonnull source_array = - cast(act.results()[TargetVarPosInResult]); + const auto* source_array = + cast(act.results()[TargetVarPosInResult]); auto end_index = static_cast(source_array->elements().size()); if (end_index == 0) { @@ -1692,11 +1694,10 @@ auto Interpreter::StepStmt() -> ErrorOr { &cast(stmt).variable_declaration())); } if (act.pos() == 2) { - Nonnull loop_var = - cast( - act.results()[LoopVarPosInResult]); - Nonnull source_array = - cast(act.results()[TargetVarPosInResult]); + const auto* loop_var = + cast(act.results()[LoopVarPosInResult]); + const auto* source_array = + cast(act.results()[TargetVarPosInResult]); auto start_index = cast(act.results()[CurrentIndexPosInResult])->value(); @@ -1714,11 +1715,10 @@ auto Interpreter::StepStmt() -> ErrorOr { cast(act.results()[EndIndexPosInResult])->value(); if (current_index < end_index) { - Nonnull source_array = + const auto* source_array = cast(act.results()[TargetVarPosInResult]); - Nonnull loop_var = - cast( - act.results()[LoopVarPosInResult]); + const auto* loop_var = cast( + act.results()[LoopVarPosInResult]); CARBON_ASSIGN_OR_RETURN( Nonnull assigned_array_element, @@ -2003,7 +2003,7 @@ auto Interpreter::StepDeclaration() -> ErrorOr { auto Interpreter::StepCleanUp() -> ErrorOr { Action& act = todo_.CurrentAction(); - CleanupAction& cleanup = cast(act); + auto& cleanup = cast(act); if (act.pos() < cleanup.locals_count()) { auto lvalue = act.scope()->locals()[cleanup.locals_count() - act.pos() - 1]; SourceLocation source_loc("destructor", 1); diff --git a/explorer/interpreter/pattern_analysis.h b/explorer/interpreter/pattern_analysis.h index ef0da52dca2d..9b8bbb84e495 100644 --- a/explorer/interpreter/pattern_analysis.h +++ b/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(nullptr)), type_(nullptr) {} void Set(Nonnull pattern); diff --git a/explorer/interpreter/type_checker.cpp b/explorer/interpreter/type_checker.cpp index f738f6c00cb3..cff3892ca039 100644 --- a/explorer/interpreter/type_checker.cpp +++ b/explorer/interpreter/type_checker.cpp @@ -27,7 +27,6 @@ using llvm::cast; using llvm::dyn_cast; -using llvm::dyn_cast_or_null; using llvm::isa; namespace Carbon { @@ -44,7 +43,7 @@ static void SetValue(Nonnull pattern, Nonnull value) { auto TypeChecker::IsSameType(Nonnull type1, Nonnull type2, - const ImplScope& impl_scope) const -> bool { + const ImplScope& /*impl_scope*/) const -> bool { return TypeEqual(type1, type2, std::nullopt); } @@ -1076,7 +1075,7 @@ class TypeChecker::ConstraintTypeBuilder { Nonnull self_binding) : self_binding_(PrepareSelfBinding(arena, self_binding)), impl_binding_(AddImplBinding(arena, self_binding_)) {} - ConstraintTypeBuilder(Nonnull arena, + ConstraintTypeBuilder(Nonnull /*arena*/, Nonnull self_binding, Nonnull impl_binding) : self_binding_(self_binding), impl_binding_(impl_binding) {} @@ -1329,7 +1328,6 @@ class TypeChecker::ConstraintTypeBuilder { return impl_binding; } - private: Nonnull self_binding_; Nonnull impl_binding_; std::vector impl_constraints_; @@ -1406,7 +1404,7 @@ auto TypeChecker::Substitute(const Bindings& bindings, return type; } - auto SubstituteIntoBindings = + auto substitute_into_bindings = [&](Nonnull inner_bindings) -> Nonnull { BindingMap values; for (const auto& [name, value] : inner_bindings->args()) { @@ -1508,14 +1506,14 @@ auto TypeChecker::Substitute(const Bindings& bindings, Nonnull new_class_type = arena_->New( &class_type.declaration(), - SubstituteIntoBindings(&class_type.bindings())); + substitute_into_bindings(&class_type.bindings())); return new_class_type; } case Value::Kind::InterfaceType: { const auto& iface_type = cast(*type); Nonnull new_iface_type = arena_->New( &iface_type.declaration(), - SubstituteIntoBindings(&iface_type.bindings())); + substitute_into_bindings(&iface_type.bindings())); return new_iface_type; } case Value::Kind::ConstraintType: { @@ -1564,7 +1562,8 @@ auto TypeChecker::Substitute(const Bindings& bindings, case Value::Kind::ImplWitness: { const auto& witness = cast(*type); return arena_->New( - &witness.declaration(), SubstituteIntoBindings(&witness.bindings())); + &witness.declaration(), + substitute_into_bindings(&witness.bindings())); } case Value::Kind::BindingWitness: { auto it = @@ -1695,9 +1694,9 @@ auto TypeChecker::MatchImpl(const InterfaceType& iface, } auto TypeChecker::MakeConstraintWitness( - const ConstraintType& constraint, + const ConstraintType& /*constraint*/, std::vector> impl_constraint_witnesses, - SourceLocation source_loc) const -> Nonnull { + SourceLocation /*source_loc*/) const -> Nonnull { return arena_->New(std::move(impl_constraint_witnesses)); } @@ -1769,7 +1768,7 @@ auto TypeChecker::DeduceCallBindings( CallExpression& call, Nonnull params_type, llvm::ArrayRef generic_params, llvm::ArrayRef> deduced_bindings, - llvm::ArrayRef> impl_bindings, + llvm::ArrayRef> /*impl_bindings*/, const ImplScope& impl_scope) -> ErrorOr { llvm::ArrayRef> params = cast(*params_type).elements(); @@ -1838,7 +1837,7 @@ auto TypeChecker::LookupInConstraint(SourceLocation source_loc, // constraints. continue; } - const InterfaceType& iface_type = cast(*lookup.context); + const auto& iface_type = cast(*lookup.context); if (std::optional> member = FindMember(member_name, iface_type.declaration().members()); member.has_value()) { @@ -2284,7 +2283,7 @@ auto TypeChecker::TypeCheckExp(Nonnull e, << " does not have a field named " << access.member_name(); } case Value::Kind::ChoiceType: { - const ChoiceType& choice = cast(*type); + const auto& choice = cast(*type); std::optional> parameter_types = choice.FindAlternative(access.member_name()); if (!parameter_types.has_value()) { @@ -2310,8 +2309,7 @@ auto TypeChecker::TypeCheckExp(Nonnull e, return Success(); } case Value::Kind::NominalClassType: { - const NominalClassType& class_type = - cast(*type); + const auto& class_type = cast(*type); CARBON_ASSIGN_OR_RETURN( auto type_member, FindMixedMemberAndType( @@ -2445,7 +2443,7 @@ auto TypeChecker::TypeCheckExp(Nonnull e, access.set_impl(impl); } - auto SubstituteIntoMemberType = [&]() { + auto substitute_into_member_type = [&]() { Nonnull member_type = &member_name.member().type(); if (member_name.interface()) { Nonnull iface_type = *member_name.interface(); @@ -2465,7 +2463,7 @@ auto TypeChecker::TypeCheckExp(Nonnull e, : DeclarationKind::VariableDeclaration) { case DeclarationKind::VariableDeclaration: if (has_instance) { - access.set_static_type(SubstituteIntoMemberType()); + access.set_static_type(substitute_into_member_type()); access.set_value_category(access.object().value_category()); return Success(); } @@ -2477,14 +2475,14 @@ auto TypeChecker::TypeCheckExp(Nonnull e, CARBON_CHECK(!has_instance || is_instance_member || !member_name.base_type().has_value()) << "vacuous compound member access"; - access.set_static_type(SubstituteIntoMemberType()); + access.set_static_type(substitute_into_member_type()); access.set_value_category(ValueCategory::Let); return Success(); } break; } case DeclarationKind::AssociatedConstantDeclaration: - access.set_static_type(SubstituteIntoMemberType()); + access.set_static_type(substitute_into_member_type()); access.set_value_category(access.object().value_category()); return Success(); default: @@ -2839,7 +2837,7 @@ auto TypeChecker::TypeCheckExp(Nonnull e, // TODO: Remove Print special casing once we have variadics or // overloads. Here, that's the name Print instead of __intrinsic_print // in errors. - if (args.size() < 1 || args.size() > 2) { + if (args.empty() || args.size() > 2) { return ProgramError(e->source_loc()) << "Print takes 1 or 2 arguments, received " << args.size(); } @@ -3411,9 +3409,10 @@ auto TypeChecker::TypeCheckPattern( } CARBON_RETURN_IF_ERROR(TypeCheckPattern( field, expected_field_type, impl_scope, enclosing_value_category)); - if (trace_stream_) + if (trace_stream_) { **trace_stream_ << "finished checking tuple pattern field " << *field << "\n"; + } field_types.push_back(&field->static_type()); } tuple.set_static_type(arena_->New(std::move(field_types))); @@ -3433,7 +3432,7 @@ auto TypeChecker::TypeCheckPattern( return ProgramError(alternative.source_loc()) << "alternative pattern does not name a choice type."; } - const ChoiceType& choice_type = cast(*type); + const auto& choice_type = cast(*type); if (expected) { CARBON_RETURN_IF_ERROR(ExpectType(alternative.source_loc(), "alternative pattern", &choice_type, @@ -3926,8 +3925,9 @@ auto TypeChecker::TypeCheckCallableDeclaration(Nonnull f, function_scope.AddParent(&impl_scope); BringImplsIntoScope(cast(f->static_type()).impl_bindings(), function_scope); - if (trace_stream_) + if (trace_stream_) { **trace_stream_ << function_scope; + } CARBON_RETURN_IF_ERROR(TypeCheckStmt(*f->body(), function_scope)); if (!f->return_term().is_omitted()) { CARBON_RETURN_IF_ERROR( @@ -4330,7 +4330,7 @@ auto TypeChecker::CheckImplIsDeducible( SourceLocation source_loc, Nonnull impl_type, Nonnull impl_iface, llvm::ArrayRef> deduced_bindings, - const ImplScope& impl_scope) -> ErrorOr { + const ImplScope& /*impl_scope*/) -> ErrorOr { ArgumentDeduction deduction(source_loc, "impl", deduced_bindings, trace_stream_); CARBON_RETURN_IF_ERROR(deduction.Deduce(impl_type, impl_type, @@ -4348,7 +4348,7 @@ auto TypeChecker::CheckImplIsDeducible( auto TypeChecker::CheckImplIsComplete(Nonnull iface_type, Nonnull impl_decl, Nonnull self_type, - Nonnull self_witness, + Nonnull /*self_witness*/, Nonnull iface_witness, const ImplScope& impl_scope) -> ErrorOr { @@ -4663,7 +4663,7 @@ auto TypeChecker::TypeCheckChoiceDeclaration( return Success(); } -static bool IsValidTypeForAliasTarget(Nonnull type) { +static auto IsValidTypeForAliasTarget(Nonnull type) -> bool { switch (type->kind()) { case Value::Kind::IntValue: case Value::Kind::FunctionValue: diff --git a/explorer/interpreter/type_checker.h b/explorer/interpreter/type_checker.h index beb24ec6789f..898fc23095c1 100644 --- a/explorer/interpreter/type_checker.h +++ b/explorer/interpreter/type_checker.h @@ -40,7 +40,7 @@ class TypeChecker { // Construct a type that is the same as `type` except that occurrences // of type variables (aka. `GenericBinding` and references to `ImplBinding`) // are replaced by their corresponding type or witness in `dict`. - auto Substitute(const Bindings& dict, Nonnull type) const + auto Substitute(const Bindings& bindings, Nonnull type) const -> Nonnull; // If `impl` can be an implementation of interface `iface` for the given @@ -60,7 +60,7 @@ class TypeChecker { auto FindMixedMemberAndType(SourceLocation source_loc, const std::string_view& name, llvm::ArrayRef> members, - const Nonnull enclosing_type) + Nonnull enclosing_type) -> ErrorOr, Nonnull>>>; @@ -300,7 +300,7 @@ class TypeChecker { // Type check all the members of the implementation. auto TypeCheckImplDeclaration(Nonnull impl_decl, - const ImplScope& impl_scope) + const ImplScope& enclosing_scope) -> ErrorOr; // This currently does nothing, but perhaps that will change in the future. diff --git a/explorer/interpreter/value.cpp b/explorer/interpreter/value.cpp index bc016b0e00ee..079336784802 100644 --- a/explorer/interpreter/value.cpp +++ b/explorer/interpreter/value.cpp @@ -20,7 +20,6 @@ namespace Carbon { using llvm::cast; using llvm::dyn_cast; using llvm::dyn_cast_or_null; -using llvm::isa; auto StructValue::FindField(std::string_view name) const -> std::optional> { @@ -39,7 +38,7 @@ static auto GetMember(Nonnull arena, Nonnull v, std::string_view f = field.name(); if (field.witness().has_value()) { - Nonnull witness = cast(*field.witness()); + auto witness = cast(*field.witness()); // Associated constants. if (auto* assoc_const = dyn_cast_or_null( @@ -104,7 +103,7 @@ static auto GetMember(Nonnull arena, Nonnull v, << " or its " << class_type; } else if ((*func)->declaration().is_method()) { // Found a method. Turn it into a bound method. - const FunctionValue& m = cast(**func); + const auto& m = cast(**func); return arena->New(&m.declaration(), me_value, &class_type.bindings()); } else { @@ -125,7 +124,7 @@ static auto GetMember(Nonnull arena, Nonnull v, } case Value::Kind::NominalClassType: { // Access a class function. - const NominalClassType& class_type = cast(*v); + const auto& class_type = cast(*v); std::optional> fun = class_type.FindFunction(f); if (fun == std::nullopt) { @@ -178,7 +177,7 @@ static auto SetFieldImpl( return arena->New(elements); } case Value::Kind::NominalClassValue: { - const NominalClassValue& object = cast(*value); + const auto& object = cast(*value); CARBON_ASSIGN_OR_RETURN(Nonnull inits, SetFieldImpl(arena, &object.inits(), path_begin, path_end, field_value, source_loc)); @@ -207,7 +206,7 @@ auto Value::SetField(Nonnull arena, const FieldPath& path, Nonnull field_value, SourceLocation source_loc) const -> ErrorOr> { - return SetFieldImpl(arena, Nonnull(this), + return SetFieldImpl(arena, static_cast>(this), path.components_.begin(), path.components_.end(), field_value, source_loc); } @@ -287,14 +286,14 @@ void Value::Print(llvm::raw_ostream& out) const { out << (cast(*this).value() ? "true" : "false"); break; case Value::Kind::DestructorValue: { - const DestructorValue& destructor = cast(*this); + const auto& destructor = cast(*this); out << "destructor [ "; out << destructor.declaration().me_pattern(); out << " ]"; break; } case Value::Kind::FunctionValue: { - const FunctionValue& fun = cast(*this); + const auto& fun = cast(*this); out << "fun<" << fun.declaration().name() << ">"; if (!fun.type_args().empty()) { out << "["; @@ -315,7 +314,7 @@ void Value::Print(llvm::raw_ostream& out) const { break; } case Value::Kind::BoundMethodValue: { - const BoundMethodValue& method = cast(*this); + const auto& method = cast(*this); out << "bound_method<" << method.declaration().name() << ">"; if (!method.type_args().empty()) { out << "["; diff --git a/explorer/interpreter/value.h b/explorer/interpreter/value.h index 69c5e4d1d935..4959fe8daa7b 100644 --- a/explorer/interpreter/value.h +++ b/explorer/interpreter/value.h @@ -119,7 +119,7 @@ class Value { // Returns whether the fully-resolved kind that this value will eventually have // is currently unknown, because it depends on a generic parameter. -inline bool IsValueKindDependent(Nonnull type) { +inline auto IsValueKindDependent(Nonnull type) -> bool { return type->kind() == Value::Kind::VariableType || type->kind() == Value::Kind::AssociatedConstant; } @@ -352,8 +352,8 @@ class AlternativeConstructorValue : public Value { AlternativeConstructorValue(std::string_view alt_name, std::string_view choice_name) : Value(Kind::AlternativeConstructorValue), - alt_name_(std::move(alt_name)), - choice_name_(std::move(choice_name)) {} + alt_name_(alt_name), + choice_name_(choice_name) {} static auto classof(const Value* value) -> bool { return value->kind() == Kind::AlternativeConstructorValue; @@ -373,8 +373,8 @@ class AlternativeValue : public Value { AlternativeValue(std::string_view alt_name, std::string_view choice_name, Nonnull argument) : Value(Kind::AlternativeValue), - alt_name_(std::move(alt_name)), - choice_name_(std::move(choice_name)), + alt_name_(alt_name), + choice_name_(choice_name), argument_(argument) {} static auto classof(const Value* value) -> bool { @@ -398,7 +398,7 @@ class TupleValue : public Value { static auto Empty() -> Nonnull { static const TupleValue empty = TupleValue(std::vector>()); - return Nonnull(&empty); + return static_cast>(&empty); } explicit TupleValue(std::vector> elements) @@ -788,7 +788,6 @@ class ConstraintType : public Value { Nonnull context; }; - public: explicit ConstraintType(Nonnull self_binding, std::vector impl_constraints, std::vector equality_constraints, diff --git a/migrate_cpp/output_segment.h b/migrate_cpp/output_segment.h index 18b61db441a6..ffb0da048f09 100644 --- a/migrate_cpp/output_segment.h +++ b/migrate_cpp/output_segment.h @@ -37,7 +37,7 @@ class OutputSegment { // instead. However, most other types we intend to support as they become // necessary. template - static constexpr bool IsSupportedClangASTNodeType() { + static constexpr auto IsSupportedClangASTNodeType() -> bool { return std::is_convertible_v || std::is_convertible_v; } @@ -62,7 +62,7 @@ class OutputSegment { friend struct OutputWriter; template - T& AssertNotNull(T* ptr) { + auto AssertNotNull(T* ptr) -> T& { CARBON_CHECK(ptr != nullptr); return *ptr; } diff --git a/migrate_cpp/rewriter.cpp b/migrate_cpp/rewriter.cpp index e37be521dd45..cac60c81813e 100644 --- a/migrate_cpp/rewriter.cpp +++ b/migrate_cpp/rewriter.cpp @@ -16,18 +16,18 @@ auto OutputWriter::Write(clang::SourceLocation loc, const OutputSegment& segment) const -> bool { return std::visit( [&](auto& content) { - using type = std::decay_t; + using Type = std::decay_t; auto [begin, end] = bounds; - if constexpr (std::is_same_v) { + if constexpr (std::is_same_v) { auto begin_offset = source_manager.getDecomposedLoc(loc).second; // Append the string replacement if the node being replaced falls // within `bounds`. if (begin <= begin_offset && begin_offset < end) { output.append(content); } - } else if constexpr (std::is_same_v || - std::is_same_v) { + } else if constexpr (std::is_same_v || + std::is_same_v) { auto content_loc = content.getSourceRange().getBegin(); auto begin_offset = source_manager.getDecomposedLoc(content_loc).second; @@ -54,7 +54,7 @@ auto OutputWriter::Write(clang::SourceLocation loc, } } } else { - static_assert(std::is_void_v, + static_assert(std::is_void_v, "Failed to handle a case in the `std::variant`."); } return true; diff --git a/migrate_cpp/rewriter.h b/migrate_cpp/rewriter.h index 3e5d0802fe6e..aeb2e8b69f4a 100644 --- a/migrate_cpp/rewriter.h +++ b/migrate_cpp/rewriter.h @@ -22,10 +22,14 @@ namespace Carbon { namespace Internal { struct Empty { - friend bool operator==(Empty, Empty) { return true; } + friend auto operator==(Empty /*unused*/, Empty /*unused*/) -> bool { + return true; + } }; struct Tombstone { - friend bool operator==(Tombstone, Tombstone) { return true; } + friend auto operator==(Tombstone /*unused*/, Tombstone /*unused*/) -> bool { + return true; + } }; // Type alias for the variant representing any of the values that can be @@ -36,16 +40,16 @@ using KeyType = // `KeyInfo` is used as a template argument to `llvm::DenseMap` to specify how // to equality-compare and hash `KeyType`. struct KeyInfo { - static bool isEqual(const KeyType& lhs, const KeyType& rhs) { + static auto isEqual(const KeyType& lhs, const KeyType& rhs) -> bool { return lhs == rhs; } - static unsigned getHashValue(const KeyType& x) { + static auto getHashValue(const KeyType& x) -> unsigned { return std::visit( [](auto x) -> unsigned { - using type = std::decay_t; - if constexpr (std::is_same_v) { + using Type = std::decay_t; + if constexpr (std::is_same_v) { return clang::DynTypedNode::DenseMapInfo::getHashValue(x); - } else if constexpr (std::is_same_v) { + } else if constexpr (std::is_same_v) { // TODO: Improve this. return reinterpret_cast(x.getTypePtr()); } else { @@ -55,8 +59,8 @@ struct KeyInfo { x); } - static KeyType getEmptyKey() { return Empty{}; } - static KeyType getTombstoneKey() { return Tombstone{}; } + static auto getEmptyKey() -> KeyType { return Empty{}; } + static auto getTombstoneKey() -> KeyType { return Tombstone{}; } }; } // namespace Internal @@ -177,7 +181,7 @@ class MigrationConsumer : public clang::ASTConsumer { public: explicit MigrationConsumer(std::string& result, std::pair output_range) - : result_(result), output_range_(output_range) {} + : result_(result), output_range_(std::move(output_range)) {} auto HandleTranslationUnit(clang::ASTContext& context) -> void override; @@ -199,11 +203,12 @@ class MigrationAction : public clang::ASTFrontendAction { // `output_range.second` will be written. explicit MigrationAction(std::string& result, std::pair output_range) - : result_(result), output_range_(output_range) {} + : result_(result), output_range_(std::move(output_range)) {} // Returns a `std::unique_ptr` to a `clang::MigrationConsumer` which populates // the output `result`. - auto CreateASTConsumer(clang::CompilerInstance&, llvm::StringRef) + auto CreateASTConsumer(clang::CompilerInstance& /*CI*/, + llvm::StringRef /*InFile*/) -> std::unique_ptr override { return std::make_unique(result_, output_range_); } diff --git a/migrate_cpp/rewriter_test.cpp b/migrate_cpp/rewriter_test.cpp index 3ce59fffec7a..c373d26d9ff6 100644 --- a/migrate_cpp/rewriter_test.cpp +++ b/migrate_cpp/rewriter_test.cpp @@ -19,7 +19,7 @@ namespace { // an annotated range. class Annotations { public: - Annotations(llvm::StringRef annotated_source) { + explicit Annotations(llvm::StringRef annotated_source) { size_t index = annotated_source.find("$[["); if (index == llvm::StringRef::npos) { source_code_ = std::string(annotated_source); @@ -39,11 +39,13 @@ class Annotations { } // Returns a view into the unannotated source. - llvm::StringRef source() const { return source_code_; } + auto source() const -> llvm::StringRef { return source_code_; } // Returns the offsets in the file representing the annotated range if they // exist and `{0, std::numeric_limits::max()}` otherwise. - std::pair range() const { return std::pair(start_, end_); } + auto range() const -> std::pair { + return std::pair(start_, end_); + } private: std::string source_code_; diff --git a/scripts/create_compdb.py b/scripts/create_compdb.py index 6eea53cd8875..b62054746183 100755 --- a/scripts/create_compdb.py +++ b/scripts/create_compdb.py @@ -57,7 +57,7 @@ print("Building compilation database...") # stand-alone files. This is a bit simpler than scraping the actual compile # actions and allows us to directly index header-only libraries easily and # pro-actively index the specific headers in the project. -source_files_query = subprocess.run( +source_files_query = subprocess.check_output( [ bazel, "query", @@ -67,11 +67,9 @@ source_files_query = subprocess.run( "--incompatible_display_source_file_location", 'filter(".*\\.(h|cpp|cc|c|cxx)$", kind("source file", deps(//...)))', ], - check=True, - stdout=subprocess.PIPE, stderr=subprocess.DEVNULL, universal_newlines=True, -).stdout +) source_files = [ Path(line.split(":")[0]) for line in source_files_query.splitlines() ] @@ -98,7 +96,7 @@ print( # Now collect the generated file labels. # cc_proto_library generates files, but they aren't seen with "generated file". -generated_file_labels = subprocess.run( +generated_file_labels = subprocess.check_output( [ bazel, "query", @@ -111,23 +109,21 @@ generated_file_labels = subprocess.run( 'kind("cc_proto_library", deps(//...))' ), ], - check=True, - stdout=subprocess.PIPE, stderr=subprocess.DEVNULL, universal_newlines=True, -).stdout.splitlines() +).splitlines() print("Found %d generated files..." % (len(generated_file_labels),)) # Directly build these labels so that indexing can find them. Allow this to # fail in case there are build errors in the client, and just warn the user # that they may be missing generated files. print("Building the generated files so that tools can find them...") -subprocess.run([bazel, "build", "--keep_going"] + generated_file_labels) +subprocess.check_call([bazel, "build", "--keep_going"] + generated_file_labels) # Also build some specific targets that depend on external packages so those are # fetched and linked into the Bazel execution root. We try to use cheap files # where possible, but in some cases need to create a virtual include directory. -subprocess.run( +subprocess.check_call( [ bazel, "build", @@ -137,6 +133,8 @@ subprocess.run( "@com_google_googletest//:LICENSE", "@com_googlesource_code_re2//:LICENSE", "@com_github_google_benchmark//:benchmark", + "@com_google_libprotobuf_mutator//:LICENSE", + "@com_google_protobuf//:any_proto", ] ) diff --git a/scripts/run_clang_tidy.py b/scripts/run_clang_tidy.py index dfba93707bd6..d5bc5721ba97 100755 --- a/scripts/run_clang_tidy.py +++ b/scripts/run_clang_tidy.py @@ -1,7 +1,6 @@ #!/usr/bin/env python3 -"""Runs clang-tidy over all Carbon files. -""" +"""Runs clang-tidy over all Carbon files.""" __copyright__ = """ Part of the Carbon Language project, under the Apache License v2.0 with LLVM @@ -9,33 +8,53 @@ Exceptions. See /LICENSE for license information. SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception """ +import argparse import os +import re import subprocess -import sys from pathlib import Path def main() -> None: + parser = argparse.ArgumentParser(__doc__) + # Copied from run-clang-tidy.py for forwarding. + parser.add_argument("-fix", action="store_true", help="Apply fix-its") + # Local flags. + parser.add_argument("files", nargs="*", help="Files to fix") + parsed_args = parser.parse_args() + + # If files are passed in, resolve them; otherwise, add a path filter. + if parsed_args.files: + files = [str(Path(f).resolve()) for f in parsed_args.files] + else: + files = ["^(?!.*/(bazel-|third_party)).*$"] + # Set the repo root as the working directory. - os.chdir(Path(__file__).parent.parent) + os.chdir(Path(__file__).resolve().parent.parent) # Ensure create_compdb has been run. subprocess.check_call(["./scripts/create_compdb.py"]) - args = sys.argv[1:] - if not args or args[0] == "--fix": - args.append("^(?!.*/(bazel-|third_party)).*$") + # Use the run-clang-tidy version that should be with the rest of the clang + # toolchain. This exposes us to version skew with user-installed clang + # versions, but avoids version skew between the script and clang-tidy + # itself. + with Path( + "./bazel-execroot/external/bazel_cc_toolchain/" + "clang_detected_variables.bzl" + ).open() as f: + clang_vars = f.read() + clang_bindir_match = re.search(r"clang_bindir = \"(.*)\"", clang_vars) + assert clang_bindir_match is not None, clang_vars + + args = [str(Path(clang_bindir_match[1]).joinpath("run-clang-tidy"))] + + # Forward flags. + if parsed_args.fix: + args.append("-fix") # Run clang-tidy from clang-tools-extra. - exit( - subprocess.call( - [ - "./bazel-execroot/external/llvm-project/clang-tools-extra/" - "clang-tidy/tool/run-clang-tidy.py", - ] - + args - ) - ) + exit(subprocess.call(args + files)) if __name__ == "__main__": diff --git a/third_party/libprotobuf_mutator/BUILD.txt b/third_party/libprotobuf_mutator/BUILD.txt index 02bf90e50ab6..c8bcadfd84d4 100644 --- a/third_party/libprotobuf_mutator/BUILD.txt +++ b/third_party/libprotobuf_mutator/BUILD.txt @@ -5,6 +5,8 @@ # libprotobuf_mutator uses cmake and doesn't provide a bazel BUILD file. # See https://github.com/google/libprotobuf-mutator/issues/91. +exports_files(["LICENSE"]) + cc_library( name = "libprotobuf_mutator", srcs = glob( @@ -16,7 +18,7 @@ cc_library( exclude = ["**/*_test.cc"], ), hdrs = ["src/libfuzzer/libfuzzer_macro.h"], - include_prefix = "libprotobuf_mutator", + strip_include_prefix = "src", visibility = ["//visibility:public"], deps = ["@com_google_protobuf//:protobuf"], ) diff --git a/toolchain/lexer/token_kind_test.cpp b/toolchain/lexer/token_kind_test.cpp index 590551bde472..74dcc9dcf88d 100644 --- a/toolchain/lexer/token_kind_test.cpp +++ b/toolchain/lexer/token_kind_test.cpp @@ -19,7 +19,7 @@ using ::testing::MatchesRegex; // We restrict symbols to punctuation characters that are expected to be widely // available on modern keyboards used for programming. constexpr llvm::StringLiteral SymbolRegex = - "[\\[\\]{}!@#%^&*()/?\\\\|;:.,<>=+~-]+"; + R"([\[\]{}!@#%^&*()/?\\|;:.,<>=+~-]+)"; // We restrict keywords to be lowercase ASCII letters and underscores. constexpr llvm::StringLiteral KeywordRegex = "[a-z_]+"; diff --git a/toolchain/semantics/node_ref.h b/toolchain/semantics/node_ref.h index 7ccb146dc30f..2f3fadab1581 100644 --- a/toolchain/semantics/node_ref.h +++ b/toolchain/semantics/node_ref.h @@ -16,7 +16,7 @@ namespace Carbon::Semantics { struct NodeStoreIndex { explicit NodeStoreIndex(int32_t index) : index(index) {} - operator int32_t() const { return index; } + explicit operator int32_t() const { return index; } int32_t index; };