[clang-format] Change LLVM style to BreakAfterAttributes == ABS_Leave

[Endilll]

This patch addresses some examples of bad formatting in Clang. The following commit contains only changes suggested by clang-format: 21689b5. I believe it's a net negative on readability, with a couple of particularly bad cases. Highlights:

-    [[clang::preferred_type(bool)]]
-    mutable unsigned CachedLocalOrUnnamed : 1;
+    [[clang::preferred_type(bool)]] mutable unsigned CachedLocalOrUnnamed : 1;

-    [[clang::preferred_type(TypeDependence)]]
-    unsigned Dependence : llvm::BitWidth<TypeDependence>;
+    [[clang::preferred_type(TypeDependence)]] unsigned Dependence
+        : llvm::BitWidth<TypeDependence>;

-    [[clang::preferred_type(ExceptionSpecificationType)]]
-    unsigned ExceptionSpecType : 4;
+    [[clang::preferred_type(
+        ExceptionSpecificationType)]] unsigned ExceptionSpecType : 4;

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-format

Author: Vlad Serebrennikov (Endilll)

Changes

This patch addresses some example of bad formatting in Clang. The following commit contains only changes suggested by clang-format: 21689b5. I believe it's a net negative on readability, with a couple of particularly bad cases. Highlights:

-    [[clang::preferred_type(bool)]]
-    mutable unsigned CachedLocalOrUnnamed : 1;
+    [[clang::preferred_type(bool)]] mutable unsigned CachedLocalOrUnnamed : 1;

-    [[clang::preferred_type(TypeDependence)]]
-    unsigned Dependence : llvm::BitWidth<TypeDependence>;
+    [[clang::preferred_type(TypeDependence)]] unsigned Dependence
+        : llvm::BitWidth<TypeDependence>;

-    [[clang::preferred_type(ExceptionSpecificationType)]]
-    unsigned ExceptionSpecType : 4;
+    [[clang::preferred_type(
+        ExceptionSpecificationType)]] unsigned ExceptionSpecType : 4;

---

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

3 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+1)
• (modified) clang/lib/Format/Format.cpp (+1-1)
• (modified) clang/unittests/Format/FormatTest.cpp (+3-4)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 7238386231e1a28..6cd5e3e077e6f3c 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -794,6 +794,7 @@ clang-format
 ------------
 - Add ``AllowBreakBeforeNoexceptSpecifier`` option.
 - Add ``AllowShortCompoundRequirementOnASingleLine`` option.
+- Change ``BreakAfterAttributes`` from ``Never`` to ``Leave`` in LLVM style
 
 libclang
 --------
diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp
index edb33f4af4defef..e1abcac5604a410 100644
--- a/clang/lib/Format/Format.cpp
+++ b/clang/lib/Format/Format.cpp
@@ -1476,7 +1476,7 @@ FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
                              /*SplitEmptyFunction=*/true,
                              /*SplitEmptyRecord=*/true,
                              /*SplitEmptyNamespace=*/true};
-  LLVMStyle.BreakAfterAttributes = FormatStyle::ABS_Never;
+  LLVMStyle.BreakAfterAttributes = FormatStyle::ABS_Leave;
   LLVMStyle.BreakAfterJavaFieldAnnotations = false;
   LLVMStyle.BreakArrays = true;
   LLVMStyle.BreakBeforeBinaryOperators = FormatStyle::BOS_None;
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index b2d84f2ee389551..5cec49a9173e824 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -26179,7 +26179,7 @@ TEST_F(FormatTest, RemoveSemicolon) {
 
 TEST_F(FormatTest, BreakAfterAttributes) {
   FormatStyle Style = getLLVMStyle();
-  EXPECT_EQ(Style.BreakAfterAttributes, FormatStyle::ABS_Never);
+  EXPECT_EQ(Style.BreakAfterAttributes, FormatStyle::ABS_Leave);
 
   constexpr StringRef Code("[[nodiscard]] inline int f(int &i);\n"
                            "[[foo([[]])]] [[nodiscard]]\n"
@@ -26193,7 +26193,9 @@ TEST_F(FormatTest, BreakAfterAttributes) {
                            "  i = 0;\n"
                            "  return 1;\n"
                            "}");
+  verifyNoChange(Code, Style);
 
+  Style.BreakAfterAttributes = FormatStyle::ABS_Never;
   verifyFormat("[[nodiscard]] inline int f(int &i);\n"
                "[[foo([[]])]] [[nodiscard]] int g(int &i);\n"
                "[[nodiscard]] inline int f(int &i) {\n"
@@ -26206,9 +26208,6 @@ TEST_F(FormatTest, BreakAfterAttributes) {
                "}",
                Code, Style);
 
-  Style.BreakAfterAttributes = FormatStyle::ABS_Leave;
-  verifyNoChange(Code, Style);
-
   Style.BreakAfterAttributes = FormatStyle::ABS_Always;
   verifyFormat("[[nodiscard]]\n"
                "inline int f(int &i);\n"

[AaronBallman]

I'm in favor of these changes. The current formatting makes some pretty bad decisions in places, but I think it's a hard problem to solve -- some attributes are longer and may want to be on their own line, other attributes are shorter and maybe are reasonable to inline, but there will always be the ones in the middle where it could go either way and will be wrong periodically. Picking "leave them where the developer put them" seems like a pretty reasonable compromise to me.

Uncertain how others feel about it though.

[owenca]

This would change the default for every other predefined style, which might be ok if there's no mention of breaking after C++11 attributes in those styles.

[Endilll]

Mozilla, WebKit, GCC, Microsoft, and Chromium doesn't appear to be opinionated on attributes at all.

Google is the only one mentioning attributes:

Attributes, and macros that expand to attributes, appear at the very beginning of the function declaration or definition, before the return type:

 ABSL_ATTRIBUTE_NOINLINE void ExpensiveFunction();
 [[nodiscard]] bool IsOk();

I'm not sure which BreakAfterAttributes option this boils down to, but I don't mind setting it back to Never for Google style if reviewers would like me to do so.

[owenca]

Google is the only one mentioning attributes:

Attributes, and macros that expand to attributes, appear at the very beginning of the function declaration or definition, before the return type:

 ABSL_ATTRIBUTE_NOINLINE void ExpensiveFunction();
 [[nodiscard]] bool IsOk();

I'm not sure which BreakAfterAttributes option this boils down to, but I don't mind setting it back to Never for Google style if reviewers would like me to do so.

I think Leave would work.