diff --git a/.clang-tidy b/.clang-tidy index 58eac2144fbf..eaf3b92c0d9b 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -13,20 +13,15 @@ Checks: - '-*' - 'bugprone-*' - 'google-*' + - 'misc-*' - 'modernize-*' - 'performance-*' - 'readability-*' - # `misc` is selectively enabled because we want a minority of them. - - 'misc-definitions-in-headers' - - 'misc-misplaced-const' - - 'misc-redundant-expression' - - 'misc-static-assert' - - 'misc-unconventional-assign-operator' - - 'misc-uniqueptr-reset-release' - - 'misc-unused-*' - # Disabled due to the implied style choices. + - '-misc-const-correctness' + - '-misc-include-cleaner' + - '-misc-use-anonymous-namespace' - '-modernize-return-braced-init-list' - '-modernize-use-default-member-init' - '-modernize-use-integer-sign-comparison' @@ -68,6 +63,11 @@ Checks: - '-google-readability-function-size' # Suggests usernames on TODOs, which we don't want. - '-google-readability-todo' + # Even with `IgnoreClassesWithAllMemberVariablesBeingPublic` to allow structs, + # we use `protected` members in too many tests. + - '-misc-non-private-member-variables-in-classes' + # Overlaps with `-Wno-missing-prototypes`. + - '-misc-use-internal-linkage' # Suggests `std::array`, which we could migrate to, but conflicts with the # status quo. - '-modernize-avoid-c-arrays' diff --git a/common/command_line.cpp b/common/command_line.cpp index 95134bed0aa1..59a893731787 100644 --- a/common/command_line.cpp +++ b/common/command_line.cpp @@ -10,6 +10,10 @@ #include "llvm/ADT/PointerIntPair.h" #include "llvm/Support/FormatVariadic.h" +// Recursion is used for subcommands. This should be okay since recursion is +// limited by command line architecture. +// NOLINTBEGIN(misc-no-recursion) + namespace Carbon::CommandLine { auto operator<<(llvm::raw_ostream& output, ParseResult result) @@ -1311,41 +1315,41 @@ void ArgBuilder::HelpHidden(bool is_help_hidden) { ArgBuilder::ArgBuilder(Arg* arg) : arg_(arg) {} void FlagBuilder::Default(bool flag_value) { - arg_->has_default = true; - arg_->default_flag = flag_value; + arg()->has_default = true; + arg()->default_flag = flag_value; } -void FlagBuilder::Set(bool* flag) { arg_->flag_storage = flag; } +void FlagBuilder::Set(bool* flag) { arg()->flag_storage = flag; } void IntegerArgBuilder::Default(int integer_value) { - arg_->has_default = true; - arg_->default_integer = integer_value; + arg()->has_default = true; + arg()->default_integer = integer_value; } void IntegerArgBuilder::Set(int* integer) { - arg_->is_append = false; - arg_->integer_storage = integer; + arg()->is_append = false; + arg()->integer_storage = integer; } void IntegerArgBuilder::Append(llvm::SmallVectorImpl* sequence) { - arg_->is_append = true; - arg_->integer_sequence = sequence; + arg()->is_append = true; + arg()->integer_sequence = sequence; } void StringArgBuilder::Default(llvm::StringRef string_value) { - arg_->has_default = true; - arg_->default_string = string_value; + arg()->has_default = true; + arg()->default_string = string_value; } void StringArgBuilder::Set(llvm::StringRef* string) { - arg_->is_append = false; - arg_->string_storage = string; + arg()->is_append = false; + arg()->string_storage = string; } void StringArgBuilder::Append( llvm::SmallVectorImpl* sequence) { - arg_->is_append = true; - arg_->string_sequence = sequence; + arg()->is_append = true; + arg()->string_sequence = sequence; } static auto IsValidName(llvm::StringRef name) -> bool { @@ -1525,3 +1529,5 @@ auto Parse(llvm::ArrayRef unparsed_args, } } // namespace Carbon::CommandLine + +// NOLINTEND(misc-no-recursion) diff --git a/common/command_line.h b/common/command_line.h index 2656675f5454..62303b339b10 100644 --- a/common/command_line.h +++ b/common/command_line.h @@ -307,10 +307,13 @@ class ArgBuilder { void MetaAction(T action); protected: - friend CommandBuilder; + friend class CommandBuilder; // `arg` must not be null. explicit ArgBuilder(Arg* arg); + auto arg() -> Arg* { return arg_; } + + private: Arg* arg_; }; @@ -756,7 +759,7 @@ auto OneOfArgBuilder::OneOfValue(llvm::StringRef str, T value) template void OneOfArgBuilder::SetOneOf(const OneOfValueT (&values)[N], T* result) { static_assert(N > 0, "Must include at least one value."); - arg_->is_append = false; + arg()->is_append = false; OneOfImpl( values, [result](T value) { *result = value; }, std::make_index_sequence{}); @@ -766,7 +769,7 @@ template void OneOfArgBuilder::AppendOneOf(const OneOfValueT (&values)[N], llvm::SmallVectorImpl* sequence) { static_assert(N > 0, "Must include at least one value."); - arg_->is_append = true; + arg()->is_append = true; OneOfImpl( values, [sequence](T value) { sequence->push_back(value); }, std::make_index_sequence{}); @@ -792,12 +795,12 @@ void OneOfArgBuilder::OneOfImpl(const OneOfValueT (&input_values)[N], // Directly copy the value strings into a heap-allocated array in the // argument. - new (&arg_->value_strings) + new (&arg()->value_strings) llvm::OwningArrayRef(value_strings); // And build a type-erased action that maps a specific value string to a value // by index. - new (&arg_->value_action) Arg::ValueActionT( + new (&arg()->value_action) Arg::ValueActionT( [values, match](const Arg& arg, llvm::StringRef value_string) -> bool { for (int i : llvm::seq(0, N)) { if (value_string == arg.value_strings[i]) { @@ -810,11 +813,11 @@ void OneOfArgBuilder::OneOfImpl(const OneOfValueT (&input_values)[N], // Fold over all the input values to see if there is a default. if ((input_values[Indices].is_default || ...)) { - CARBON_CHECK(!arg_->is_append, "Can't append default."); + CARBON_CHECK(!arg()->is_append, "Can't append default."); CARBON_CHECK((input_values[Indices].is_default + ... + 0) == 1, "Cannot default more than one value."); - arg_->has_default = true; + arg()->has_default = true; // First build a lambda that configures the default using an index. We'll // call this below, this lambda isn't the one that is stored. @@ -822,12 +825,12 @@ void OneOfArgBuilder::OneOfImpl(const OneOfValueT (&input_values)[N], // Now that we have the desired default index, build a lambda and store it // as the default action. This lambda is stored and so it captures the // necessary information explicitly and by value. - new (&arg_->default_action) + new (&arg()->default_action) Arg::DefaultActionT([value = default_value.value, match](const Arg& /*arg*/) { match(value); }); // Also store the index itself for use when printing help. - arg_->default_value_index = index; + arg()->default_value_index = index; }; // Now we fold across the inputs and in the one case that is the default, we diff --git a/migrate_cpp/cpp_refactoring/fn_inserter.cpp b/migrate_cpp/cpp_refactoring/fn_inserter.cpp index 76558b649a8a..f370ffce160e 100644 --- a/migrate_cpp/cpp_refactoring/fn_inserter.cpp +++ b/migrate_cpp/cpp_refactoring/fn_inserter.cpp @@ -6,6 +6,7 @@ #include "clang/ASTMatchers/ASTMatchers.h" +// NOLINTNEXTLINE(readability-identifier-naming) namespace cam = ::clang::ast_matchers; namespace Carbon { diff --git a/migrate_cpp/cpp_refactoring/for_range.cpp b/migrate_cpp/cpp_refactoring/for_range.cpp index 371b7dc21c42..eef4946fa0fe 100644 --- a/migrate_cpp/cpp_refactoring/for_range.cpp +++ b/migrate_cpp/cpp_refactoring/for_range.cpp @@ -6,6 +6,7 @@ #include "clang/ASTMatchers/ASTMatchers.h" +// NOLINTNEXTLINE(readability-identifier-naming) namespace cam = ::clang::ast_matchers; namespace Carbon { diff --git a/migrate_cpp/cpp_refactoring/var_decl.cpp b/migrate_cpp/cpp_refactoring/var_decl.cpp index 2f9fe9ad6be0..7e586c618ba3 100644 --- a/migrate_cpp/cpp_refactoring/var_decl.cpp +++ b/migrate_cpp/cpp_refactoring/var_decl.cpp @@ -7,6 +7,7 @@ #include "clang/ASTMatchers/ASTMatchers.h" #include "llvm/Support/FormatVariadic.h" +// NOLINTNEXTLINE(readability-identifier-naming) namespace cam = ::clang::ast_matchers; namespace Carbon { @@ -36,7 +37,6 @@ auto VarDecl::GetTypeStr(const clang::VarDecl& decl) -> std::string { auto type_loc = decl.getTypeSourceInfo()->getTypeLoc(); std::vector> segments; while (!type_loc.isNull()) { - std::string text; auto qualifiers = type_loc.getType().getLocalQualifiers(); std::string qual_str; if (!qualifiers.empty()) { diff --git a/migrate_cpp/rewriter.cpp b/migrate_cpp/rewriter.cpp index 55636b74a57e..35f0e7c6b227 100644 --- a/migrate_cpp/rewriter.cpp +++ b/migrate_cpp/rewriter.cpp @@ -297,6 +297,7 @@ auto RewriteBuilder::VisitUnaryOperator(clang::UnaryOperator* expr) -> bool { return true; } +// NOLINTNEXTLINE(misc-no-recursion): Recursion may be okay for migration. auto RewriteBuilder::TraverseFunctionDecl(clang::FunctionDecl* decl) -> bool { clang::TypeLoc return_type_loc = decl->getFunctionTypeLoc().getReturnLoc(); if (!TraverseTypeLoc(return_type_loc)) { @@ -345,6 +346,7 @@ auto RewriteBuilder::TraverseFunctionDecl(clang::FunctionDecl* decl) -> bool { return true; } +// NOLINTNEXTLINE(misc-no-recursion): Recursion may be okay for migration. auto RewriteBuilder::TraverseVarDecl(clang::VarDecl* decl) -> bool { clang::TypeLoc loc = decl->getTypeSourceInfo()->getTypeLoc(); if (!TraverseTypeLoc(loc)) { diff --git a/testing/fuzzing/proto_to_carbon.cpp b/testing/fuzzing/proto_to_carbon.cpp index a9cb40ebc00b..6fef9ad804aa 100644 --- a/testing/fuzzing/proto_to_carbon.cpp +++ b/testing/fuzzing/proto_to_carbon.cpp @@ -12,6 +12,9 @@ #include "llvm/Support/raw_ostream.h" #include "testing/fuzzing/carbon.pb.h" +// This is a test file, so the recursion is okay. +// NOLINTBEGIN(misc-no-recursion) + namespace Carbon { static auto ExpressionToCarbon(const Fuzzing::Expression& expression, @@ -1023,3 +1026,5 @@ auto ParseCarbonTextProto(const std::string& contents) } } // namespace Carbon + +// NOLINTEND(misc-no-recursion) diff --git a/toolchain/base/value_ids.h b/toolchain/base/value_ids.h index 465e8db65257..fc4ee04be61b 100644 --- a/toolchain/base/value_ids.h +++ b/toolchain/base/value_ids.h @@ -21,8 +21,7 @@ namespace Carbon { // // These values are not canonicalized, because we don't expect them to repeat // and don't use them in SemIR values. -class Real : public Printable { - public: +struct Real : public Printable { auto Print(llvm::raw_ostream& output_stream) const -> void { mantissa.print(output_stream, /*isSigned=*/false); output_stream << "*" << (is_decimal ? "10" : "2") << "^" << exponent; diff --git a/toolchain/check/convert.cpp b/toolchain/check/convert.cpp index 507548c3bccb..9e57ebde42cc 100644 --- a/toolchain/check/convert.cpp +++ b/toolchain/check/convert.cpp @@ -22,6 +22,10 @@ #include "toolchain/sem_ir/inst.h" #include "toolchain/sem_ir/typed_insts.h" +// TODO: This contains a lot of recursion. Consider removing it in order to +// prevent accidents. +// NOLINTBEGIN(misc-no-recursion) + namespace Carbon::Check { // Given an initializing expression, find its return slot argument. Returns @@ -1245,3 +1249,5 @@ auto ExprAsType(Context& context, SemIR::LocId loc_id, SemIR::InstId value_id) } } // namespace Carbon::Check + +// NOLINTEND(misc-no-recursion) diff --git a/toolchain/check/import_ref.cpp b/toolchain/check/import_ref.cpp index 26b0b9576b69..fe8a1a1655ff 100644 --- a/toolchain/check/import_ref.cpp +++ b/toolchain/check/import_ref.cpp @@ -342,6 +342,14 @@ class ImportContext { } protected: + auto pending_generics() -> llvm::SmallVector& { + return pending_generics_; + } + auto pending_specifics() -> llvm::SmallVector& { + return pending_specifics_; + } + + private: Context& context_; SemIR::ImportIRId import_ir_id_; const SemIR::File& import_ir_; @@ -2526,6 +2534,10 @@ static auto TryResolveTypedInst(ImportRefResolver& resolver, // Invalid if more has been added to the stack. This is the same as // TryResolveInst, except that it may resolve symbolic constants as canonical // constants instead of as constants associated with a particular generic. +// +// TODO: Consider refactoring the body to a helper in order to eliminate +// recursion. +// NOLINTNEXTLINE(misc-no-recursion) static auto TryResolveInstCanonical(ImportRefResolver& resolver, SemIR::InstId inst_id, SemIR::ConstantId const_id) @@ -2850,7 +2862,7 @@ static auto FinishPendingSpecific(ImportRefResolver& resolver, auto ImportRefResolver::PerformPendingWork() -> void { // Note that the individual Finish steps can add new pending work, so keep // going until we have no more work to do. - while (!pending_generics_.empty() || !pending_specifics_.empty()) { + while (!pending_generics().empty() || !pending_specifics().empty()) { // Process generics in the order that we added them because a later // generic might refer to an earlier one, and the calls to // RebuildGenericEvalBlock assume that the reachable SemIR is in a valid @@ -2858,13 +2870,13 @@ auto ImportRefResolver::PerformPendingWork() -> void { // TODO: Import the generic eval block rather than calling // RebuildGenericEvalBlock to rebuild it so that order doesn't matter. // NOLINTNEXTLINE(modernize-loop-convert) - for (size_t i = 0; i != pending_generics_.size(); ++i) { - FinishPendingGeneric(*this, pending_generics_[i]); + for (size_t i = 0; i != pending_generics().size(); ++i) { + FinishPendingGeneric(*this, pending_generics()[i]); } - pending_generics_.clear(); + pending_generics().clear(); - while (!pending_specifics_.empty()) { - FinishPendingSpecific(*this, pending_specifics_.pop_back_val()); + while (!pending_specifics().empty()) { + FinishPendingSpecific(*this, pending_specifics().pop_back_val()); } } } diff --git a/toolchain/check/subst.cpp b/toolchain/check/subst.cpp index 70766ce9eec7..965d1b77837d 100644 --- a/toolchain/check/subst.cpp +++ b/toolchain/check/subst.cpp @@ -61,6 +61,19 @@ class Worklist { // Pushes the specified operand onto the worklist. static auto PushOperand(Context& context, Worklist& worklist, SemIR::IdKind kind, int32_t arg) -> void { + auto push_block = [&](SemIR::InstBlockId block_id) { + for (auto inst_id : + context.inst_blocks().Get(SemIR::InstBlockId(block_id))) { + worklist.Push(inst_id); + } + }; + + auto push_specific = [&](SemIR::SpecificId specific_id) { + if (specific_id.is_valid()) { + push_block(context.specifics().Get(specific_id).args_id); + } + }; + switch (kind) { case SemIR::IdKind::For: if (SemIR::InstId inst_id(arg); inst_id.is_valid()) { @@ -73,9 +86,7 @@ static auto PushOperand(Context& context, Worklist& worklist, } break; case SemIR::IdKind::For: - for (auto inst_id : context.inst_blocks().Get(SemIR::InstBlockId(arg))) { - worklist.Push(inst_id); - } + push_block(SemIR::InstBlockId(arg)); break; case SemIR::IdKind::For: { for (auto field : @@ -90,18 +101,13 @@ static auto PushOperand(Context& context, Worklist& worklist, } break; case SemIR::IdKind::For: - if (auto specific_id = static_cast(arg); - specific_id.is_valid()) { - PushOperand(context, worklist, SemIR::IdKind::For, - context.specifics().Get(specific_id).args_id.index); - } + push_specific(SemIR::SpecificId(arg)); break; case SemIR::IdKind::For: { const auto& facet_type_info = context.facet_types().Get(SemIR::FacetTypeId(arg)); for (auto interface : facet_type_info.impls_constraints) { - PushOperand(context, worklist, SemIR::IdKind::For, - interface.specific_id.index); + push_specific(interface.specific_id); } for (auto rewrite : facet_type_info.rewrite_constraints) { auto lhs_inst_id = @@ -134,6 +140,25 @@ static auto ExpandOperands(Context& context, Worklist& worklist, // Pops the specified operand from the worklist and returns it. static auto PopOperand(Context& context, Worklist& worklist, SemIR::IdKind kind, int32_t arg) -> int32_t { + auto pop_block_id = [&](SemIR::InstBlockId old_inst_block_id) { + auto size = context.inst_blocks().Get(old_inst_block_id).size(); + SemIR::CopyOnWriteInstBlock new_inst_block(context.sem_ir(), + old_inst_block_id); + for (auto i : llvm::reverse(llvm::seq(size))) { + new_inst_block.Set(i, worklist.Pop()); + } + return new_inst_block.GetCanonical(); + }; + + auto pop_specific = [&](SemIR::SpecificId specific_id) { + if (!specific_id.is_valid()) { + return specific_id; + } + auto& specific = context.specifics().Get(specific_id); + auto args_id = pop_block_id(specific.args_id); + return context.specifics().GetOrAdd(specific.generic_id, args_id); + }; + switch (kind) { case SemIR::IdKind::For: { SemIR::InstId inst_id(arg); @@ -150,14 +175,7 @@ static auto PopOperand(Context& context, Worklist& worklist, SemIR::IdKind kind, return context.GetTypeIdForTypeInst(worklist.Pop()).index; } case SemIR::IdKind::For: { - SemIR::InstBlockId old_inst_block_id(arg); - auto size = context.inst_blocks().Get(old_inst_block_id).size(); - SemIR::CopyOnWriteInstBlock new_inst_block(context.sem_ir(), - old_inst_block_id); - for (auto i : llvm::reverse(llvm::seq(size))) { - new_inst_block.Set(i, worklist.Pop()); - } - return new_inst_block.GetCanonical().index; + return pop_block_id(SemIR::InstBlockId(arg)).index; } case SemIR::IdKind::For: { SemIR::StructTypeFieldsId old_fields_id(arg); @@ -182,15 +200,7 @@ static auto PopOperand(Context& context, Worklist& worklist, SemIR::IdKind kind, return new_type_block.GetCanonical().index; } case SemIR::IdKind::For: { - SemIR::SpecificId specific_id(arg); - if (!specific_id.is_valid()) { - return arg; - } - auto& specific = context.specifics().Get(specific_id); - auto args_id = SemIR::InstBlockId( - PopOperand(context, worklist, SemIR::IdKind::For, - specific.args_id.index)); - return context.specifics().GetOrAdd(specific.generic_id, args_id).index; + return pop_specific(SemIR::SpecificId(arg)).index; } case SemIR::IdKind::For: { const auto& old_facet_type_info = @@ -206,11 +216,8 @@ static auto PopOperand(Context& context, Worklist& worklist, SemIR::IdKind kind, } for (auto i : llvm::reverse( llvm::seq(old_facet_type_info.impls_constraints.size()))) { - auto specific_id = PopOperand( - context, worklist, SemIR::IdKind::For, - old_facet_type_info.impls_constraints[i].specific_id.index); new_facet_type_info.impls_constraints[i].specific_id = - SemIR::SpecificId(specific_id); + pop_specific(old_facet_type_info.impls_constraints[i].specific_id); } new_facet_type_info.Canonicalize(); return context.facet_types().Add(new_facet_type_info).index; diff --git a/toolchain/sem_ir/formatter.cpp b/toolchain/sem_ir/formatter.cpp index 6ee487912caf..48e136932bc0 100644 --- a/toolchain/sem_ir/formatter.cpp +++ b/toolchain/sem_ir/formatter.cpp @@ -20,6 +20,11 @@ #include "toolchain/sem_ir/name_scope.h" #include "toolchain/sem_ir/typed_insts.h" +// TODO: Consider addressing recursion here, although it's not critical because +// the formatter isn't required to work on arbitrary code. Still, it may help +// in the future to debug complex code. +// NOLINTBEGIN(misc-no-recursion) + namespace Carbon::SemIR { // Formatter for printing textual Semantics IR. @@ -1383,3 +1388,5 @@ auto Formatter::Print(llvm::raw_ostream& out) -> void { } } // namespace Carbon::SemIR + +// NOLINTEND(misc-no-recursion) diff --git a/toolchain/sem_ir/inst_namer.cpp b/toolchain/sem_ir/inst_namer.cpp index a33e2b57add1..b7a0f81c5b88 100644 --- a/toolchain/sem_ir/inst_namer.cpp +++ b/toolchain/sem_ir/inst_namer.cpp @@ -355,6 +355,9 @@ auto InstNamer::AddBlockLabel(ScopeId scope_id, SemIR::LocId loc_id, AddBlockLabel(scope_id, branch.target_id, name.str(), loc_id); } +// TODO: Consider addressing recursion here. It may be important because +// InstNamer is used for debug info. +// NOLINTNEXTLINE(misc-no-recursion) auto InstNamer::CollectNamesInBlock(ScopeId scope_id, InstBlockId block_id) -> void { if (block_id.is_valid()) { @@ -362,6 +365,7 @@ auto InstNamer::CollectNamesInBlock(ScopeId scope_id, InstBlockId block_id) } } +// NOLINTNEXTLINE(misc-no-recursion): See above TODO. auto InstNamer::CollectNamesInBlock(ScopeId scope_id, llvm::ArrayRef block) -> void { Scope& scope = GetScopeInfo(scope_id); diff --git a/toolchain/testing/yaml_test_helpers.cpp b/toolchain/testing/yaml_test_helpers.cpp index 4eff3f2a19e2..8bb9b877395c 100644 --- a/toolchain/testing/yaml_test_helpers.cpp +++ b/toolchain/testing/yaml_test_helpers.cpp @@ -9,6 +9,8 @@ namespace Carbon::Testing::Yaml { +// This is for tests, so we should be okay with the recursion here. +// NOLINTNEXTLINE(misc-no-recursion) static auto Parse(llvm::yaml::Node* node) -> Value { CARBON_CHECK(node != nullptr);