[clang] [clang-format] Honor AllowShortFunctionsOnASingleLine.Other w/o Empty (PR #235369)

[via cfe-commits]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang-format

Author: Theia Dobyns (theia-ajax)

<details>
<summary>Changes</summary>

Other only took effect through isAll(), so {Empty: false, Other: true} merged no functions.  
Merge non-empty functions whenever Other is set and leave empty ones to Empty and Inline.

```
clang-format --style="{BasedOnStyle: LLVM, AllowShortFunctionsOnASingleLine: {Empty: false, Other: true}}"
```

Before:
```
void f() {
}
int g() {
  return 0;
}
```

After:
```
void f() {
}
int g() { return 0; }
```

Empty is now checked through one empty-body test shared with Other, which covers both wrapped and unwrapped braces.  
It combines the existing Empty check with the wrapped-brace check in the AfterFunction merge path, which is now redundant.

Fixes #<!-- -->206144

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


2 Files Affected:

- (modified) clang/lib/Format/UnwrappedLineFormatter.cpp (+16-9) 
- (modified) clang/unittests/Format/FormatTest.cpp (+43) 


``````````diff
diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp b/clang/lib/Format/UnwrappedLineFormatter.cpp
index d1f68877ef0339..84ad693a63f4f7 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -337,17 +337,27 @@ class LineJoiner {
       }
     }
 
+    // The line after the left brace of the function, which may be wrapped
+    // onto its own line.
+    const bool LBraceWrapped = NextLine.First->is(TT_FunctionLBrace) &&
+                               NextLine.First == NextLine.Last;
+    const AnnotatedLine *BodyLine =
+        LBraceWrapped ? (I + 2 != E ? I[2] : nullptr) : &NextLine;
+    const bool IsEmptyFunction = BodyLine && BodyLine->First->is(tok::r_brace);
+
     auto ShouldMergeShortFunctions = [&] {
       if (Style.AllowShortFunctionsOnASingleLine.isAll())
         return true;
 
-      if (Style.AllowShortFunctionsOnASingleLine.Empty &&
-          NextLine.First->is(tok::r_brace)) {
+      if (Style.AllowShortFunctionsOnASingleLine.Empty && IsEmptyFunction)
+        return true;
+
+      // Other covers only non-empty functions. Empty functions are left to
+      // Empty and Inline.
+      if (Style.AllowShortFunctionsOnASingleLine.Other && !IsEmptyFunction)
         return true;
-      }
 
-      if (Style.AllowShortFunctionsOnASingleLine.Inline &&
-          !Style.AllowShortFunctionsOnASingleLine.Other) {
+      if (Style.AllowShortFunctionsOnASingleLine.Inline) {
         if (Style.isJavaScript() && TheLine->Last->is(TT_FunctionLBrace))
           return true;
 
@@ -574,10 +584,7 @@ class LineJoiner {
       Limit -= 2;
 
       unsigned MergedLines = 0;
-      if (MergeShortFunctions ||
-          (Style.AllowShortFunctionsOnASingleLine.Empty &&
-           NextLine.First == NextLine.Last && I + 2 != E &&
-           I[2]->First->is(tok::r_brace))) {
+      if (MergeShortFunctions) {
         MergedLines = tryMergeSimpleBlock(I + 1, E, Limit);
         // If we managed to merge the block, count the function header, which is
         // on a separate line.
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index f38287757ffba1..ef367116acd829 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -15605,6 +15605,49 @@ TEST_F(FormatTest, CustomShortFunctionOptions) {
                "  return 42;\n"
                "}",
                CustomMixed);
+
+  // Test with Other = true, Empty = false
+  FormatStyle CustomOther = getLLVMStyle();
+  CustomOther.AllowShortFunctionsOnASingleLine = {/*Empty=*/false,
+                                                  /*Inline=*/false,
+                                                  /*Other=*/true};
+
+  // Non-empty functions should be on a single line
+  verifyFormat("int f() { return 42; }", CustomOther);
+  verifyFormat("class C {\n"
+               "  int f() { return 42; }\n"
+               "};",
+               CustomOther);
+
+  // Empty functions should be multi-line
+  verifyFormat("int f() {\n"
+               "}",
+               CustomOther);
+  verifyFormat("class C {\n"
+               "  int f() {\n"
+               "  }\n"
+               "};",
+               CustomOther);
+
+  // Test with AfterFunction = true
+  CustomOther.BreakBeforeBraces = FormatStyle::BS_Custom;
+  CustomOther.BraceWrapping.AfterFunction = true;
+  verifyFormat("int f() { return 42; }", CustomOther);
+  verifyFormat("int f()\n"
+               "{\n"
+               "}",
+               CustomOther);
+
+  // Inline = true still merges empty functions in a class
+  CustomOther.AllowShortFunctionsOnASingleLine.Inline = true;
+  verifyFormat("class C {\n"
+               "  int f() {}\n"
+               "};",
+               CustomOther);
+  verifyFormat("int f()\n"
+               "{\n"
+               "}",
+               CustomOther);
 }
 
 TEST_F(FormatTest, PullInlineOnlyFunctionDefinitionsIntoSingleLine) {

``````````

</details>


https://github.com/llvm/llvm-project/pull/235369