From cb4686bf217f946a88c814d0fa5171cf3846d34b Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Wed, 18 Dec 2024 16:31:41 -0800 Subject: [PATCH] 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 | 25 ++++++++----- docs/project/cpp_style_guide.md | 5 +++ .../cpp_refactoring/matcher_test_base.h | 19 +++++----- toolchain/check/node_stack.h | 2 +- .../diagnostics/diagnostic_emitter_test.cpp | 2 +- toolchain/driver/driver_test.cpp | 2 +- toolchain/install/busybox_info_test.cpp | 2 +- toolchain/install/install_paths_test.cpp | 2 +- toolchain/lex/string_literal_test.cpp | 36 +++++++++---------- toolchain/lex/tokenized_buffer_test.cpp | 2 +- toolchain/parse/typed_nodes_test.cpp | 2 +- 11 files changed, 56 insertions(+), 43 deletions(-) diff --git a/.clang-tidy b/.clang-tidy index 45a3cfe0cf55..f5484bfcd6c5 100644 --- a/.clang-tidy +++ b/.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 diff --git a/docs/project/cpp_style_guide.md b/docs/project/cpp_style_guide.md index 1079d8d4b30d..8409ca4379a2 100644 --- a/docs/project/cpp_style_guide.md +++ b/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 diff --git a/migrate_cpp/cpp_refactoring/matcher_test_base.h b/migrate_cpp/cpp_refactoring/matcher_test_base.h index 3adaf6642241..81c38972ec28 100644 --- a/migrate_cpp/cpp_refactoring/matcher_test_base.h +++ b/migrate_cpp/cpp_refactoring/matcher_test_base.h @@ -19,25 +19,25 @@ namespace Carbon::Testing { template class MatcherTestBase : public ::testing::Test { protected: - MatcherTestBase() : matchers(&replacements) { - matchers.Register(std::make_unique()); + MatcherTestBase() : matchers_(&replacements_) { + matchers_.Register(std::make_unique()); } // 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::tooling::FileContentMappings())); - EXPECT_THAT(replacements, testing::ElementsAre(testing::Key(Filename))); + EXPECT_THAT(replacements_, testing::ElementsAre(testing::Key(Filename))); llvm::Expected 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 diff --git a/toolchain/check/node_stack.h b/toolchain/check/node_stack.h index 7184ecb213d5..0de0e75b0aa5 100644 --- a/toolchain/check/node_stack.h +++ b/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 diff --git a/toolchain/diagnostics/diagnostic_emitter_test.cpp b/toolchain/diagnostics/diagnostic_emitter_test.cpp index d6e704b77be4..043d5d708aeb 100644 --- a/toolchain/diagnostics/diagnostic_emitter_test.cpp +++ b/toolchain/diagnostics/diagnostic_emitter_test.cpp @@ -25,7 +25,7 @@ struct FakeDiagnosticConverter : DiagnosticConverter { }; class DiagnosticEmitterTest : public ::testing::Test { - protected: + public: DiagnosticEmitterTest() : emitter_(converter_, consumer_) {} FakeDiagnosticConverter converter_; diff --git a/toolchain/driver/driver_test.cpp b/toolchain/driver/driver_test.cpp index d4aba3d65e34..3e6fd0d77944 100644 --- a/toolchain/driver/driver_test.cpp +++ b/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())), diff --git a/toolchain/install/busybox_info_test.cpp b/toolchain/install/busybox_info_test.cpp index 5299307bca20..1d01bb735183 100644 --- a/toolchain/install/busybox_info_test.cpp +++ b/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"); diff --git a/toolchain/install/install_paths_test.cpp b/toolchain/install/install_paths_test.cpp index 9a633f2d76a4..1ba4abfc6eb9 100644 --- a/toolchain/install/install_paths_test.cpp +++ b/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)); diff --git a/toolchain/lex/string_literal_test.cpp b/toolchain/lex/string_literal_test.cpp index 82db0679b7f4..32819568c238 100644 --- a/toolchain/lex/string_literal_test.cpp +++ b/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 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 emitter(converter, error_tracker); - return token.ComputeValue(allocator, emitter); + DiagnosticEmitter 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); } diff --git a/toolchain/lex/tokenized_buffer_test.cpp b/toolchain/lex/tokenized_buffer_test.cpp index fb642bd24013..b842d1e979c4 100644 --- a/toolchain/lex/tokenized_buffer_test.cpp +++ b/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_; }; diff --git a/toolchain/parse/typed_nodes_test.cpp b/toolchain/parse/typed_nodes_test.cpp index 076dbb045fb2..48a9d68a583f 100644 --- a/toolchain/parse/typed_nodes_test.cpp +++ b/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_;