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_;