[clang-format] Add common attribute macros to Google style

[ilya-biryukov]

We have found that 199fc97 regresses formatting of our codebases because we do not properly configure the names of attribute macros.

GUARDED_BY and ABSL_GUARDED_BY are very commoon in Google codebases so it is reasonable to include them by default to avoid the need for extra configuration in every Google repository.

[llvmbot]

@llvm/pr-subscribers-clang-format

Author: Ilya Biryukov (ilya-biryukov)

Changes

We have found that 199fc97 regresses formatting of our codebases because we do not properly configure the names of attribute macros.

GUARDED_BY and ABSL_GUARDED_BY are very commoon in Google codebases so it is reasonable to include them by default to avoid the need for extra configuration in every Google repository.

---

Full diff: https://github.com/llvm/llvm-project/pull/76239.diff

2 Files Affected:

• (modified) clang/lib/Format/Format.cpp (+3)
• (modified) clang/unittests/Format/FormatTest.cpp (+24-3)

diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp
index 28271181e07d0c..f798d555bf9929 100644
--- a/clang/lib/Format/Format.cpp
+++ b/clang/lib/Format/Format.cpp
@@ -1698,6 +1698,9 @@ FormatStyle getGoogleStyle(FormatStyle::LanguageKind Language) {
           /*BasedOnStyle=*/"google",
       },
   };
+  GoogleStyle.AttributeMacros.push_back("GUARDED_BY");
+  GoogleStyle.AttributeMacros.push_back("ABSL_GUARDED_BY");
+
   GoogleStyle.SpacesBeforeTrailingComments = 2;
   GoogleStyle.Standard = FormatStyle::LS_Auto;
 
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index 0e08723aa9e947..9772c3be71877f 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -7,6 +7,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "FormatTestBase.h"
+#include "gmock/gmock.h"
 
 #define DEBUG_TYPE "format-test"
 
@@ -8497,7 +8498,10 @@ TEST_F(FormatTest, BreaksFunctionDeclarationsWithTrailingTokens) {
                "    __attribute__((unused));");
 
   Style = getGoogleStyle();
-  Style.AttributeMacros.push_back("GUARDED_BY");
+  ASSERT_THAT(Style.AttributeMacros,
+              testing::AllOf(testing::Contains("GUARDED_BY"),
+                             testing::Contains("ABSL_GUARDED_BY")));
+
   verifyFormat(
       "bool aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
       "    GUARDED_BY(aaaaaaaaaaaa);",
@@ -8514,6 +8518,23 @@ TEST_F(FormatTest, BreaksFunctionDeclarationsWithTrailingTokens) {
       "bool aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa GUARDED_BY(aaaaaaaaaaaa) =\n"
       "    aaaaaaaaaaaaaaaaaaaaaaaaa;",
       Style);
+
+  verifyFormat(
+      "bool aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
+      "    ABSL_GUARDED_BY(aaaaaaaaaaaa);",
+      Style);
+  verifyFormat(
+      "bool aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
+      "    ABSL_GUARDED_BY(aaaaaaaaaaaa);",
+      Style);
+  verifyFormat(
+      "bool aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa ABSL_GUARDED_BY(aaaaaaaaaaaa) =\n"
+      "    aaaaaaaa::aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa;",
+      Style);
+  verifyFormat(
+      "bool aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa ABSL_GUARDED_BY(aaaaaaaaaaaa) =\n"
+      "    aaaaaaaaaaaaaaaaaaaaaaaaa;",
+      Style);
 }
 
 TEST_F(FormatTest, FunctionAnnotations) {
@@ -10072,11 +10093,11 @@ TEST_F(FormatTest, ReturnTypeBreakingStyle) {
                getGoogleStyleWithColumns(40));
   verifyFormat("Tttttttttttttttttttttttt ppppppppppppppp\n"
                "    ABSL_GUARDED_BY(mutex1)\n"
-               "        ABSL_GUARDED_BY(mutex2);",
+               "    ABSL_GUARDED_BY(mutex2);",
                getGoogleStyleWithColumns(40));
   verifyFormat("Tttttt f(int a, int b)\n"
                "    ABSL_GUARDED_BY(mutex1)\n"
-               "        ABSL_GUARDED_BY(mutex2);",
+               "    ABSL_GUARDED_BY(mutex2);",
                getGoogleStyleWithColumns(40));
   // * typedefs
   verifyGoogleFormat("typedef ATTR(X) char x;");

[krasimirgg]

thank you!

[owenca]

GUARDED_BY and ABSL_GUARDED_BY are very commoon in Google codebases so it is reasonable to include them by default to avoid the need for extra configuration in every Google repository.

I don't feel comfortable with this as it might impact users of the Google style with non-Google codebases. @mydeveloperday @HazardyKnusperkeks @rymiel can you chime in?

[rymiel]

Who should have the final say on the Google code style if not Google?

[HazardyKnusperkeks]

Who should have the final say on the Google code style if not Google?

That's also my point of view. Who ever decides to use google style, has to live with google changing its style.

[ilya-biryukov]

This change did not aim to change the behavior, it's quite the opposite, I tried to guide clang-format parsing to recognize attribute and format the code in the way we always intended it to.

If there is a way to do that without adding the macro names to the configuration explicitly, it would definitely be preferable for us too. I just went with what I thought to be the easiest path as I don't really expect people to use the macro names for other purposes. Definitely not ABSL_GUARDED_BY, although I can see why GUARDED_BY might be more contentious.