Note namespace and static recommendations in C++ style guide (#1041)

common/check_test.cpp

@@ -6,7 +6,8 @@
 
 #include <gtest/gtest.h>
 
-namespace Carbon {
+namespace Carbon::Testing {
+namespace {
 
 TEST(CheckTest, CheckTrue) { CHECK(true); }
 
@@ -50,4 +51,5 @@ TEST(ErrorTest, FatalNoReturnRequired) {
                "FATAL failure at common/check_test.cpp:.+: msg\n");
 }
 
-}  // namespace Carbon
+}  // namespace
+}  // namespace Carbon::Testing

common/indirect_value_test.cpp

@@ -8,7 +8,7 @@
 
 #include <string>
 
-namespace Carbon {
+namespace Carbon::Testing {
 namespace {
 
 TEST(IndirectValueTest, ConstAccess) {
@@ -126,4 +126,4 @@ TEST(IndirectValueTest, IncompleteType) {
 }
 
 }  // namespace
-}  // namespace Carbon
+}  // namespace Carbon::Testing

common/string_helpers.cpp

@@ -13,13 +13,11 @@
 
 namespace Carbon {
 
-namespace {
-
-constexpr llvm::StringRef TripleQuotes = "\"\"\"";
-constexpr llvm::StringRef HorizontalWhitespaceChars = " \t";
+static constexpr llvm::StringRef TripleQuotes = "\"\"\"";
+static constexpr llvm::StringRef HorizontalWhitespaceChars = " \t";
 
 // Carbon only takes uppercase hex input.
-auto FromHex(char c) -> std::optional<char> {
+static auto FromHex(char c) -> std::optional<char> {
   if (c >= '0' && c <= '9') {
     return c - '0';
   }
@@ -30,12 +28,10 @@ auto FromHex(char c) -> std::optional<char> {
 }
 
 // Creates an error instance with the specified `message`.
-llvm::Expected<std::string> MakeError(llvm::Twine message) {
+static auto MakeError(llvm::Twine message) -> llvm::Expected<std::string> {
   return llvm::createStringError(llvm::inconvertibleErrorCode(), message);
 }
 
-}  // namespace
-
 auto UnescapeStringLiteral(llvm::StringRef source, bool is_block_string)
     -> std::optional<std::string> {
   std::string ret;

common/string_helpers_test.cpp

@@ -15,7 +15,7 @@ using ::llvm::toString;
 using ::testing::Eq;
 using ::testing::Optional;
 
-namespace Carbon {
+namespace Carbon::Testing {
 namespace {
 
 TEST(UnescapeStringLiteral, Valid) {
@@ -197,4 +197,4 @@ TEST(ParseBlockStringLiteral, OkMultipleSlashes) {
 }
 
 }  // namespace
-}  // namespace Carbon
+}  // namespace Carbon::Testing

docs/project/cpp_style_guide.md

@@ -141,6 +141,15 @@ these.
         necessary to create a scope for a variable.
     -   Always break the line immediately after an open brace except for empty
         loop bodies.
+-   For
+    [internal linkage](https://google.github.io/styleguide/cppguide.html#Internal_Linkage)
+    of definitions of functions and variables, prefer `static` over anonymous
+    namespaces. `static` minimizes the context necessary to notice the internal
+    linkage of a definition.
+    -   Anonymous namespaces are still necessary for classes and enums.
+    -   Tests are an exception and should typically be wrapped with
+        `namespace Carbon::Testing { namespace { ... } }` to keep everything
+        internal.
 
 ### Copyable and movable types
 

executable_semantics/ast/ast_test_matchers_test.cpp

@@ -13,7 +13,7 @@
 #include "executable_semantics/ast/statement.h"
 #include "executable_semantics/common/arena.h"
 
-namespace Carbon {
+namespace Carbon::Testing {
 namespace {
 
 using ::testing::_;
@@ -159,4 +159,4 @@ TEST(ASTDeclarationsTest, BasicUsage) {
 }
 
 }  // namespace
-}  // namespace Carbon
+}  // namespace Carbon::Testing

executable_semantics/ast/expression_test.cpp

@@ -13,7 +13,7 @@
 #include "executable_semantics/common/arena.h"
 #include "llvm/Support/Casting.h"
 
-namespace Carbon {
+namespace Carbon::Testing {
 namespace {
 
 using llvm::cast;
@@ -135,4 +135,4 @@ TEST_F(ExpressionTest, BinaryAsTuple) {
 }
 
 }  // namespace
-}  // namespace Carbon
+}  // namespace Carbon::Testing

executable_semantics/ast/pattern_test.cpp

@@ -12,7 +12,7 @@
 #include "executable_semantics/common/arena.h"
 #include "llvm/Support/Casting.h"
 
-namespace Carbon {
+namespace Carbon::Testing {
 namespace {
 
 using llvm::cast;
@@ -129,4 +129,4 @@ TEST_F(PatternTest, BinaryAsTuplePattern) {
 }
 
 }  // namespace
-}  // namespace Carbon
+}  // namespace Carbon::Testing

executable_semantics/common/error_test.cpp

@@ -6,7 +6,7 @@
 
 #include <gtest/gtest.h>
 
-namespace Carbon {
+namespace Carbon::Testing {
 namespace {
 
 TEST(ErrorTest, FatalProgramError) {
@@ -30,4 +30,4 @@ TEST(ErrorTest, FatalProgramErrorLine) {
 }
 
 }  // namespace
-}  // namespace Carbon
+}  // namespace Carbon::Testing

executable_semantics/interpreter/value.cpp

@@ -27,10 +27,8 @@ auto StructValue::FindField(const std::string& name) const
   return std::nullopt;
 }
 
-namespace {
-
-auto GetMember(Nonnull<Arena*> arena, Nonnull<const Value*> v,
-               const std::string& f, SourceLocation source_loc)
+static auto GetMember(Nonnull<Arena*> arena, Nonnull<const Value*> v,
+                      const std::string& f, SourceLocation source_loc)
     -> Nonnull<const Value*> {
   switch (v->kind()) {
     case Value::Kind::StructValue: {
@@ -62,8 +60,6 @@ auto GetMember(Nonnull<Arena*> arena, Nonnull<const Value*> v,
   }
 }
 
-}  // namespace
-
 auto Value::GetField(Nonnull<Arena*> arena, const FieldPath& path,
                      SourceLocation source_loc) const -> Nonnull<const Value*> {
   Nonnull<const Value*> value(this);
@@ -73,13 +69,11 @@ auto Value::GetField(Nonnull<Arena*> arena, const FieldPath& path,
   return value;
 }
 
-namespace {
-
-auto SetFieldImpl(Nonnull<Arena*> arena, Nonnull<const Value*> value,
-                  std::vector<std::string>::const_iterator path_begin,
-                  std::vector<std::string>::const_iterator path_end,
-                  Nonnull<const Value*> field_value, SourceLocation source_loc)
-    -> Nonnull<const Value*> {
+static auto SetFieldImpl(Nonnull<Arena*> arena, Nonnull<const Value*> value,
+                         std::vector<std::string>::const_iterator path_begin,
+                         std::vector<std::string>::const_iterator path_end,
+                         Nonnull<const Value*> field_value,
+                         SourceLocation source_loc) -> Nonnull<const Value*> {
   if (path_begin == path_end) {
     return field_value;
   }
@@ -120,8 +114,6 @@ auto SetFieldImpl(Nonnull<Arena*> arena, Nonnull<const Value*> value,
   }
 }
 
-}  // namespace
-
 auto Value::SetField(Nonnull<Arena*> arena, const FieldPath& path,
                      Nonnull<const Value*> field_value,
                      SourceLocation source_loc) const -> Nonnull<const Value*> {

executable_semantics/syntax/parse_test.cpp

@@ -12,7 +12,7 @@
 
 #include "executable_semantics/common/arena.h"
 
-namespace Carbon {
+namespace Carbon::Testing {
 namespace {
 
 static constexpr std::string_view FileContents = R"(
@@ -30,4 +30,4 @@ TEST(ParseTest, ParseFromString) {
 }
 
 }  // namespace
-}  // namespace Carbon
+}  // namespace Carbon::Testing

executable_semantics/syntax/unimplemented_example_test.cpp

@@ -9,7 +9,7 @@
 #include "executable_semantics/syntax/parse.h"
 #include "executable_semantics/syntax/parse_test_matchers.h"
 
-namespace Carbon {
+namespace Carbon::Testing {
 namespace {
 
 using ::testing::ElementsAre;
@@ -33,4 +33,4 @@ TEST(UnimplementedExampleTest, VerifyPrecedence) {
 }
 
 }  // namespace
-}  // namespace Carbon
+}  // namespace Carbon::Testing

migrate_cpp/cpp_refactoring/fn_inserter_test.cpp

@@ -6,7 +6,7 @@
 
 #include "migrate_cpp/cpp_refactoring/matcher_test_base.h"
 
-namespace Carbon {
+namespace Carbon::Testing {
 namespace {
 
 class FnInserterTest : public MatcherTestBase<FnInserterFactory> {};
@@ -90,4 +90,4 @@ TEST_F(FnInserterTest, LegacyReturn) {
 }
 
 }  // namespace
-}  // namespace Carbon
+}  // namespace Carbon::Testing

migrate_cpp/cpp_refactoring/for_range_test.cpp

@@ -6,7 +6,7 @@
 
 #include "migrate_cpp/cpp_refactoring/matcher_test_base.h"
 
-namespace Carbon {
+namespace Carbon::Testing {
 namespace {
 
 class ForRangeTest : public MatcherTestBase<ForRangeFactory> {};
@@ -49,4 +49,4 @@ TEST_F(ForRangeTest, NoSpace) {
 }
 
 }  // namespace
-}  // namespace Carbon
+}  // namespace Carbon::Testing

migrate_cpp/cpp_refactoring/matcher_test_base.h

@@ -13,7 +13,7 @@
 #include "clang/Tooling/Tooling.h"
 #include "migrate_cpp/cpp_refactoring/matcher_manager.h"
 
-namespace Carbon {
+namespace Carbon::Testing {
 
 // Matcher test framework.
 template <typename MatcherFactoryType>
@@ -61,6 +61,6 @@ class MatcherTestBase : public ::testing::Test {
   MatcherManager matchers;
 };
 
-}  // namespace Carbon
+}  // namespace Carbon::Testing
 
 #endif  // MIGRATE_CPP_CPP_REFACTORING_MATCHER_TEST_BASE_H_

migrate_cpp/cpp_refactoring/var_decl_test.cpp

@@ -6,7 +6,7 @@
 
 #include "migrate_cpp/cpp_refactoring/matcher_test_base.h"
 
-namespace Carbon {
+namespace Carbon::Testing {
 namespace {
 
 class VarDeclTest : public MatcherTestBase<VarDeclFactory> {};
@@ -254,4 +254,4 @@ TEST_F(VarDeclTest, Template) {
 }
 
 }  // namespace
-}  // namespace Carbon
+}  // namespace Carbon::Testing