Enable misc-non-private-member-variables-in-classes and adjust style to match (#4702)

Pursuant to discussion regarding #4699, turn on
`misc-non-private-member-variables-in-classes` using the
`IgnoreClassesWithAllMemberVariablesBeingPublic` flag (the check treats
structs as classes, so we need this for structs with all-public
members). Updates the style guide notes to match, which should be pretty
minor due to the scoping of test fixtures.

Also fixes some underscore uses in test files on the way. Basically this
is keeping the style for [class data member
naming](https://google.github.io/styleguide/cppguide.html#Variable_Names)
even while making them public.

.clang-tidy

@@ -65,9 +65,6 @@ WarningsAsErrors: '*'
 # - '-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
@@ -110,13 +107,17 @@ Checks:
   -bugprone-macro-parentheses, -bugprone-narrowing-conversions,
   -bugprone-switch-missing-default-case, -bugprone-unchecked-optional-access,
   -google-readability-function-size, -google-readability-todo,
-  -misc-non-private-member-variables-in-classes, -misc-use-internal-linkage,
-  -modernize-avoid-c-arrays, -modernize-use-designated-initializers,
-  -modernize-use-nodiscard, -performance-unnecessary-value-param,
-  -readability-enum-initial-value, -readability-function-cognitive-complexity,
-  -readability-magic-numbers, -readability-redundant-member-init,
-  -readability-suspicious-call-argument
+  -misc-use-internal-linkage, -modernize-avoid-c-arrays,
+  -modernize-use-designated-initializers, -modernize-use-nodiscard,
+  -performance-unnecessary-value-param, -readability-enum-initial-value,
+  -readability-function-cognitive-complexity, -readability-magic-numbers,
+  -readability-redundant-member-init, -readability-suspicious-call-argument
 CheckOptions:
+  # Don't warn on structs; done by ignoring when there are only public members.
+  - key: misc-non-private-member-variables-in-classes.IgnoreClassesWithAllMemberVariablesBeingPublic
+    value: true
+
+  # CamelCase names.
   - key: readability-identifier-naming.ClassCase
     value: CamelCase
   - key: readability-identifier-naming.ClassConstantCase
@@ -135,14 +136,20 @@ CheckOptions:
     value: CamelCase
   - key: readability-identifier-naming.UnionCase
     value: CamelCase
+
+  # lower_case names.
   - key: readability-identifier-naming.ClassMemberCase
     value: lower_case
   - key: readability-identifier-naming.ParameterCase
     value: lower_case
   - key: readability-identifier-naming.VariableCase
     value: lower_case
+
+  # TODO: This is for explorer's use of LLVM casting support, so we should be
+  # able to remove it once explorer is deleted.
   - key: readability-identifier-naming.MethodIgnoredRegexp
     value: '^classof$'
+
   # This erroneously fires in C++20 mode with LLVM 16 clang-tidy, due to:
   # https://github.com/llvm/llvm-project/issues/46097
   - key: readability-identifier-naming.TemplateParameterIgnoredRegexp

docs/project/cpp_style_guide.md

@@ -184,6 +184,11 @@ these.
     -   Tests are an exception and should typically be wrapped in an anonymous
         namespace under the namespace of the code under test, to keep everything
         internal.
+-   For
+    [Access Control](https://google.github.io/styleguide/cppguide.html#Access_Control),
+    specifically for test fixtures in `.cpp` files, we use `public` instead of
+    `protected`. This is motivated by the
+    `misc-non-private-member-variables-in-classes` tidy check.
 
 ### Copyable and movable types
 

migrate_cpp/cpp_refactoring/matcher_test_base.h

@@ -19,25 +19,25 @@ namespace Carbon::Testing {
 template <typename MatcherFactoryType>
 class MatcherTestBase : public ::testing::Test {
  protected:
-  MatcherTestBase() : matchers(&replacements) {
-    matchers.Register(std::make_unique<MatcherFactoryType>());
+  MatcherTestBase() : matchers_(&replacements_) {
+    matchers_.Register(std::make_unique<MatcherFactoryType>());
   }
 
   // Expects that the replacements produced by running the finder result in
   // the specified code transformation.
   void ExpectReplacement(llvm::StringRef before, llvm::StringRef after) {
     auto factory =
-        clang::tooling::newFrontendActionFactory(matchers.GetFinder());
+        clang::tooling::newFrontendActionFactory(matchers_.GetFinder());
     constexpr char Filename[] = "test.cc";
-    replacements.clear();
-    replacements.insert({Filename, {}});
+    replacements_.clear();
+    replacements_.insert({Filename, {}});
     ASSERT_TRUE(clang::tooling::runToolOnCodeWithArgs(
         factory->create(), before, {}, Filename, "clang-tool",
         std::make_shared<clang::PCHContainerOperations>(),
         clang::tooling::FileContentMappings()));
-    EXPECT_THAT(replacements, testing::ElementsAre(testing::Key(Filename)));
+    EXPECT_THAT(replacements_, testing::ElementsAre(testing::Key(Filename)));
     llvm::Expected<std::string> actual =
-        clang::tooling::applyAllReplacements(before, replacements[Filename]);
+        clang::tooling::applyAllReplacements(before, replacements_[Filename]);
 
     // Make a specific note if the matcher didn't make any changes.
     std::string unchanged;
@@ -57,8 +57,9 @@ class MatcherTestBase : public ::testing::Test {
     }
   }
 
-  Matcher::ReplacementMap replacements;
-  MatcherManager matchers;
+ private:
+  Matcher::ReplacementMap replacements_;
+  MatcherManager matchers_;
 };
 
 }  // namespace Carbon::Testing

toolchain/check/node_stack.h

@@ -339,7 +339,7 @@ class NodeStack {
   auto empty() const -> bool { return stack_.empty(); }
   auto size() const -> size_t { return stack_.size(); }
 
- protected:
+ private:
   // An ID that can be associated with a parse node.
   //
   // Each parse node kind has a corresponding Id::Kind indicating which kind of

toolchain/diagnostics/diagnostic_emitter_test.cpp

@@ -25,7 +25,7 @@ struct FakeDiagnosticConverter : DiagnosticConverter<int> {
 };
 
 class DiagnosticEmitterTest : public ::testing::Test {
- protected:
+ public:
   DiagnosticEmitterTest() : emitter_(converter_, consumer_) {}
 
   FakeDiagnosticConverter converter_;

toolchain/driver/driver_test.cpp

@@ -40,7 +40,7 @@ static auto ReadFile(std::filesystem::path path) -> std::string {
 }
 
 class DriverTest : public testing::Test {
- protected:
+ public:
   DriverTest()
       : installation_(
             InstallPaths::MakeForBazelRunfiles(Testing::GetExePath())),

toolchain/install/busybox_info_test.cpp

@@ -18,7 +18,7 @@ namespace {
 using ::testing::Eq;
 
 class BusyboxInfoTest : public ::testing::Test {
- protected:
+ public:
   // Set up a temp directory for the test case.
   explicit BusyboxInfoTest() {
     const char* tmpdir = std::getenv("TEST_TMPDIR");

toolchain/install/install_paths_test.cpp

@@ -33,7 +33,7 @@ using ::testing::Optional;
 using ::testing::StartsWith;
 
 class InstallPathsTest : public ::testing::Test {
- protected:
+ public:
   InstallPathsTest() {
     std::string error;
     test_runfiles_.reset(Runfiles::Create(Testing::GetExePath().str(), &error));

toolchain/lex/string_literal_test.cpp

@@ -15,8 +15,8 @@ namespace Carbon::Lex {
 namespace {
 
 class StringLiteralTest : public ::testing::Test {
- protected:
-  StringLiteralTest() : error_tracker(ConsoleDiagnosticConsumer()) {}
+ public:
+  StringLiteralTest() : error_tracker_(ConsoleDiagnosticConsumer()) {}
 
   auto Lex(llvm::StringRef text) -> StringLiteral {
     std::optional<StringLiteral> result = StringLiteral::Lex(text);
@@ -28,12 +28,12 @@ class StringLiteralTest : public ::testing::Test {
   auto Parse(llvm::StringRef text) -> llvm::StringRef {
     StringLiteral token = Lex(text);
     Testing::SingleTokenDiagnosticConverter converter(text);
-    DiagnosticEmitter<const char*> emitter(converter, error_tracker);
-    return token.ComputeValue(allocator, emitter);
+    DiagnosticEmitter<const char*> emitter(converter, error_tracker_);
+    return token.ComputeValue(allocator_, emitter);
   }
 
-  llvm::BumpPtrAllocator allocator;
-  ErrorTrackingDiagnosticConsumer error_tracker;
+  llvm::BumpPtrAllocator allocator_;
+  ErrorTrackingDiagnosticConsumer error_tracker_;
 };
 
 TEST_F(StringLiteralTest, StringLiteralBounds) {
@@ -207,9 +207,9 @@ TEST_F(StringLiteralTest, StringLiteralContents) {
   };
 
   for (auto [test, expected] : testcases) {
-    error_tracker.Reset();
+    error_tracker_.Reset();
     auto value = Parse(test.trim());
-    EXPECT_FALSE(error_tracker.seen_error()) << "`" << test << "`";
+    EXPECT_FALSE(error_tracker_.seen_error()) << "`" << test << "`";
     EXPECT_EQ(value, expected);
   }
 }
@@ -238,9 +238,9 @@ TEST_F(StringLiteralTest, DoubleQuotedMultiLineLiteral) {
   };
 
   for (auto [test, contents] : testcases) {
-    error_tracker.Reset();
+    error_tracker_.Reset();
     auto value = Parse(test.trim());
-    EXPECT_TRUE(error_tracker.seen_error()) << "`" << test << "`";
+    EXPECT_TRUE(error_tracker_.seen_error()) << "`" << test << "`";
     EXPECT_EQ(value, contents);
   }
 }
@@ -262,9 +262,9 @@ TEST_F(StringLiteralTest, StringLiteralBadIndent) {
   };
 
   for (auto [test, contents] : testcases) {
-    error_tracker.Reset();
+    error_tracker_.Reset();
     auto value = Parse(test);
-    EXPECT_TRUE(error_tracker.seen_error()) << "`" << test << "`";
+    EXPECT_TRUE(error_tracker_.seen_error()) << "`" << test << "`";
     EXPECT_EQ(value, contents);
   }
 }
@@ -311,28 +311,28 @@ TEST_F(StringLiteralTest, StringLiteralBadEscapeSequence) {
   };
 
   for (llvm::StringLiteral test : testcases) {
-    error_tracker.Reset();
+    error_tracker_.Reset();
     Parse(test);
-    EXPECT_TRUE(error_tracker.seen_error()) << "`" << test << "`";
+    EXPECT_TRUE(error_tracker_.seen_error()) << "`" << test << "`";
     // TODO: Test value produced by error recovery.
   }
 }
 
 TEST_F(StringLiteralTest, TabInString) {
   auto value = Parse("\"x\ty\"");
-  EXPECT_TRUE(error_tracker.seen_error());
+  EXPECT_TRUE(error_tracker_.seen_error());
   EXPECT_EQ(value, "x\ty");
 }
 
 TEST_F(StringLiteralTest, TabAtEndOfString) {
   auto value = Parse("\"\t\t\t\"");
-  EXPECT_TRUE(error_tracker.seen_error());
+  EXPECT_TRUE(error_tracker_.seen_error());
   EXPECT_EQ(value, "\t\t\t");
 }
 
 TEST_F(StringLiteralTest, TabInBlockString) {
   auto value = Parse("'''\nx\ty\n'''");
-  EXPECT_TRUE(error_tracker.seen_error());
+  EXPECT_TRUE(error_tracker_.seen_error());
   EXPECT_EQ(value, "x\ty\n");
 }
 
@@ -341,7 +341,7 @@ TEST_F(StringLiteralTest, UnicodeTooManyDigits) {
   text.append(10000, '9');
   text.append("}");
   auto value = Parse("\"\\" + text + "\"");
-  EXPECT_TRUE(error_tracker.seen_error());
+  EXPECT_TRUE(error_tracker_.seen_error());
   EXPECT_EQ(value, text);
 }
 

toolchain/lex/tokenized_buffer_test.cpp

@@ -37,7 +37,7 @@ using ::testing::Pair;
 namespace Yaml = ::Carbon::Testing::Yaml;
 
 class LexerTest : public ::testing::Test {
- protected:
+ public:
   Testing::CompileHelper compile_helper_;
 };
 

toolchain/parse/typed_nodes_test.cpp

@@ -45,7 +45,7 @@ namespace {
 #include "toolchain/parse/node_kind.def"
 
 class TypedNodeTest : public ::testing::Test {
- protected:
+ public:
   using Peer = TypedNodesTestPeer;
 
   Testing::CompileHelper compile_helper_;