[PATCH] D99031: [clang-format] Fix CompactNamespaces corner case when AllowShortLambdasOnASingleLine/BraceWrapping.BeforeLambdaBody are set

[Ahmed Mahdy via Phabricator via cfe-commits]

aybassiouny created this revision.
aybassiouny added reviewers: curdeius, Wawha, MyDeveloperDay, HazardyKnusperkeks.
aybassiouny added a project: clang-format.
aybassiouny requested review of this revision.
Herald added a project: clang.

**Summary** `CompactNamespaces` config stops work when `AllowShortLambdasOnASingleLine` is set to false or `BraceWrapping.BeforeLambdaBody` is true

Input:

  namespace out {
  namespace in {
  } 
  } // namespace out::in

Expected output:

  namespace out { namespace in {
  }} // namespace out::in

Output from current binaries:

  namespace out {
  namespace in {
  } 
  } // namespace out::in

Config triggering the issue:

  ---
  AllowShortLambdasOnASingleLine: None
  CompactNamespaces: true
  ...

This config too triggers the same issue:

  ---
  BraceWrapping:
    BeforeLambdaBody :    true
  BreakBeforeBraces: Custom
  CompactNamespaces: true
  ...

**What's happening:** Seems there's a corner case when AllowShortLambdasOnASingleLine is false, or when `BraceWrapping.BeforeLambdaBody` is true, that causes CompactNamespaces to stop working. The root cause seems to be isAllmanLambdaBrace <https://github.com/llvm/llvm-project/blob/bcc1dee60019f3a488a04dc7f701f7a692040fed/clang/lib/Format/TokenAnnotator.cpp#L3495>, that !!assumes the namespace brace is a lambda and blocks compacting the namespace!!. The regression was probably introduced with this commit <https://github.com/llvm/llvm-project/commit/fa0118e6e588fe303b08e7e06ba28ac1f8d50c68>.

I have only added a UT covering the corner cases, please let me know if more coverage is needed, I am new to the codebase. One corner case I am aware of, is anonymous namespaces, which would not be affected by my `Tok.Previous->Previous->is(tok::kw_namespace)` check, but I am not sure what's the expected behavior there anyway.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D99031

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===================================================================
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -2589,6 +2589,33 @@
                    Style));
 }
 
+TEST_F(FormatTest, FormatsCompactNamespacesLambdaRegression) {
+  FormatStyle Style = getLLVMStyle();
+  Style.CompactNamespaces = true;
+
+  // Make sure compact namespaces are not confused with lambdas
+  auto NoShortLambdaStyle{Style};
+  NoShortLambdaStyle.AllowShortLambdasOnASingleLine = FormatStyle::SLS_None;
+  EXPECT_EQ("namespace out { namespace in {\n"
+            "}} // namespace out::in",
+            format("namespace out {\n"
+                   "namespace in {\n"
+                   "} // namespace in\n"
+                   "} // namespace out",
+                   NoShortLambdaStyle));
+
+  auto BraceWrappingBeforeLambdaBodyStyle{Style};
+  BraceWrappingBeforeLambdaBodyStyle.BreakBeforeBraces = FormatStyle::BS_Custom;
+  BraceWrappingBeforeLambdaBodyStyle.BraceWrapping.BeforeLambdaBody = true;
+  EXPECT_EQ("namespace out { namespace in {\n"
+            "}} // namespace out::in",
+            format("namespace out {\n"
+                   "namespace in {\n"
+                   "} // namespace in\n"
+                   "} // namespace out",
+                   BraceWrappingBeforeLambdaBodyStyle));
+}
+
 TEST_F(FormatTest, FormatsExternC) {
   verifyFormat("extern \"C\" {\nint a;");
   verifyFormat("extern \"C\" {}");
Index: clang/lib/Format/TokenAnnotator.cpp
===================================================================
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -3494,7 +3494,8 @@
 }
 static bool isAllmanLambdaBrace(const FormatToken &Tok) {
   return (Tok.is(tok::l_brace) && Tok.is(BK_Block) &&
-          !Tok.isOneOf(TT_ObjCBlockLBrace, TT_DictLiteral));
+          !Tok.isOneOf(TT_ObjCBlockLBrace, TT_DictLiteral) &&
+          !Tok.Previous->Previous->is(tok::kw_namespace));
 }
 
 static bool isAllmanBraceIncludedBreakableLambda(


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D99031.332146.patch
Type: text/x-patch
Size: 2079 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20210321/78ba070c/attachment.bin>