Look at flipping clang-tidy's misc-* to enable-by-default

.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'

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<int>* 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<llvm::StringRef>* 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<llvm::StringRef> unparsed_args,
 }
 
 }  // namespace Carbon::CommandLine
+
+// NOLINTEND(misc-no-recursion)

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 <typename T, typename U, size_t N>
 void OneOfArgBuilder::SetOneOf(const OneOfValueT<U> (&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<N>{});
@@ -766,7 +769,7 @@ template <typename T, typename U, size_t N>
 void OneOfArgBuilder::AppendOneOf(const OneOfValueT<U> (&values)[N],
                                   llvm::SmallVectorImpl<T>* 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<N>{});
@@ -792,12 +795,12 @@ void OneOfArgBuilder::OneOfImpl(const OneOfValueT<U> (&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<llvm::StringRef>(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<int>(0, N)) {
           if (value_string == arg.value_strings[i]) {
@@ -810,24 +813,24 @@ void OneOfArgBuilder::OneOfImpl(const OneOfValueT<U> (&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.
     auto set_default = [&](size_t index, const auto& default_value) {
       // 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

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 {

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 {

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<std::pair<clang::TypeLoc::TypeLocClass, std::string>> segments;
   while (!type_loc.isNull()) {
-    std::string text;
     auto qualifiers = type_loc.getType().getLocalQualifiers();
     std::string qual_str;
     if (!qualifiers.empty()) {

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)) {

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)

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<Real> {
- public:
+struct Real : public Printable<Real> {
   auto Print(llvm::raw_ostream& output_stream) const -> void {
     mantissa.print(output_stream, /*isSigned=*/false);
     output_stream << "*" << (is_decimal ? "10" : "2") << "^" << exponent;

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)

toolchain/check/import_ref.cpp

@@ -342,6 +342,14 @@ class ImportContext {
   }
 
  protected:
+  auto pending_generics() -> llvm::SmallVector<PendingGeneric>& {
+    return pending_generics_;
+  }
+  auto pending_specifics() -> llvm::SmallVector<PendingSpecific>& {
+    return pending_specifics_;
+  }
+
+ private:
   Context& context_;
   SemIR::ImportIRId import_ir_id_;
   const SemIR::File& import_ir_;
@@ -2523,6 +2531,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)
@@ -2847,21 +2859,21 @@ 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
     // state.
     // 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());
     }
   }
 }