release/20.x: [clang-format] Don't remove parentheses separated from ellipsis by comma (#130471)

[llvmbot]

Backport 7d4d850

Requested by: @owenca

[llvmbot]

@HazardyKnusperkeks What do you think about merging this PR to the release branch?

[llvmbot]

@llvm/pr-subscribers-clang-format

Author: None (llvmbot)

Changes

Backport 7d4d850

Requested by: @owenca

---

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

2 Files Affected:

• (modified) clang/lib/Format/UnwrappedLineParser.cpp (+51-35)
• (modified) clang/unittests/Format/FormatTest.cpp (+4)

diff --git a/clang/lib/Format/UnwrappedLineParser.cpp b/clang/lib/Format/UnwrappedLineParser.cpp
index 9b4257fdd8c8f..9a03e9409fcbc 100644
--- a/clang/lib/Format/UnwrappedLineParser.cpp
+++ b/clang/lib/Format/UnwrappedLineParser.cpp
@@ -2562,12 +2562,12 @@ bool UnwrappedLineParser::parseBracedList(bool IsAngleBracket, bool IsEnum) {
 /// Returns whether there is a `=` token between the parentheses.
 bool UnwrappedLineParser::parseParens(TokenType AmpAmpTokenType) {
   assert(FormatTok->is(tok::l_paren) && "'(' expected.");
-  auto *LeftParen = FormatTok;
+  auto *LParen = FormatTok;
   bool SeenComma = false;
   bool SeenEqual = false;
   bool MightBeFoldExpr = false;
-  const bool MightBeStmtExpr = Tokens->peekNextToken()->is(tok::l_brace);
   nextToken();
+  const bool MightBeStmtExpr = FormatTok->is(tok::l_brace);
   do {
     switch (FormatTok->Tok.getKind()) {
     case tok::l_paren:
@@ -2577,44 +2577,60 @@ bool UnwrappedLineParser::parseParens(TokenType AmpAmpTokenType) {
         parseChildBlock();
       break;
     case tok::r_paren: {
-      auto *Prev = LeftParen->Previous;
-      if (!MightBeStmtExpr && !MightBeFoldExpr && !Line->InMacroBody &&
-          Style.RemoveParentheses > FormatStyle::RPS_Leave) {
-        const auto *Next = Tokens->peekNextToken();
-        const bool DoubleParens =
-            Prev && Prev->is(tok::l_paren) && Next && Next->is(tok::r_paren);
-        const bool CommaSeparated =
-            !DoubleParens && Prev && Prev->isOneOf(tok::l_paren, tok::comma) &&
-            Next && Next->isOneOf(tok::comma, tok::r_paren);
-        const auto *PrevPrev = Prev ? Prev->getPreviousNonComment() : nullptr;
-        const bool Excluded =
-            PrevPrev &&
-            (PrevPrev->isOneOf(tok::kw___attribute, tok::kw_decltype) ||
-             SeenComma ||
-             (SeenEqual &&
-              (PrevPrev->isOneOf(tok::kw_if, tok::kw_while) ||
-               PrevPrev->endsSequence(tok::kw_constexpr, tok::kw_if))));
-        const bool ReturnParens =
-            Style.RemoveParentheses == FormatStyle::RPS_ReturnStatement &&
-            ((NestedLambdas.empty() && !IsDecltypeAutoFunction) ||
-             (!NestedLambdas.empty() && !NestedLambdas.back())) &&
-            Prev && Prev->isOneOf(tok::kw_return, tok::kw_co_return) && Next &&
-            Next->is(tok::semi);
-        if ((DoubleParens && !Excluded) || (CommaSeparated && !SeenComma) ||
-            ReturnParens) {
-          LeftParen->Optional = true;
-          FormatTok->Optional = true;
-        }
-      }
+      auto *Prev = LParen->Previous;
+      auto *RParen = FormatTok;
+      nextToken();
       if (Prev) {
+        auto OptionalParens = [&] {
+          if (MightBeStmtExpr || MightBeFoldExpr || Line->InMacroBody ||
+              SeenComma || Style.RemoveParentheses == FormatStyle::RPS_Leave) {
+            return false;
+          }
+          const bool DoubleParens =
+              Prev->is(tok::l_paren) && FormatTok->is(tok::r_paren);
+          if (DoubleParens) {
+            const auto *PrevPrev = Prev->getPreviousNonComment();
+            const bool Excluded =
+                PrevPrev &&
+                (PrevPrev->isOneOf(tok::kw___attribute, tok::kw_decltype) ||
+                 (SeenEqual &&
+                  (PrevPrev->isOneOf(tok::kw_if, tok::kw_while) ||
+                   PrevPrev->endsSequence(tok::kw_constexpr, tok::kw_if))));
+            if (!Excluded)
+              return true;
+          } else {
+            const bool CommaSeparated =
+                Prev->isOneOf(tok::l_paren, tok::comma) &&
+                FormatTok->isOneOf(tok::comma, tok::r_paren);
+            if (CommaSeparated &&
+                // LParen is not preceded by ellipsis, comma.
+                !Prev->endsSequence(tok::comma, tok::ellipsis) &&
+                // RParen is not followed by comma, ellipsis.
+                !(FormatTok->is(tok::comma) &&
+                  Tokens->peekNextToken()->is(tok::ellipsis))) {
+              return true;
+            }
+            const bool ReturnParens =
+                Style.RemoveParentheses == FormatStyle::RPS_ReturnStatement &&
+                ((NestedLambdas.empty() && !IsDecltypeAutoFunction) ||
+                 (!NestedLambdas.empty() && !NestedLambdas.back())) &&
+                Prev->isOneOf(tok::kw_return, tok::kw_co_return) &&
+                FormatTok->is(tok::semi);
+            if (ReturnParens)
+              return true;
+          }
+          return false;
+        };
         if (Prev->is(TT_TypenameMacro)) {
-          LeftParen->setFinalizedType(TT_TypeDeclarationParen);
-          FormatTok->setFinalizedType(TT_TypeDeclarationParen);
-        } else if (Prev->is(tok::greater) && FormatTok->Previous == LeftParen) {
+          LParen->setFinalizedType(TT_TypeDeclarationParen);
+          RParen->setFinalizedType(TT_TypeDeclarationParen);
+        } else if (Prev->is(tok::greater) && RParen->Previous == LParen) {
           Prev->setFinalizedType(TT_TemplateCloser);
+        } else if (OptionalParens()) {
+          LParen->Optional = true;
+          RParen->Optional = true;
         }
       }
-      nextToken();
       return SeenEqual;
     }
     case tok::r_brace:
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index d1e96e0fa544a..6508ca2e7174f 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -27881,6 +27881,10 @@ TEST_F(FormatTest, RemoveParentheses) {
   verifyFormat("foo((a, b));", "foo(((a), b));", Style);
   verifyFormat("foo((a, b));", "foo((a, (b)));", Style);
   verifyFormat("foo((a, b, c));", "foo((a, ((b)), c));", Style);
+  verifyFormat("(..., (hash_a = hash_combine(hash_a, hash_b)));",
+               "(..., ((hash_a = hash_combine(hash_a, hash_b))));", Style);
+  verifyFormat("((hash_a = hash_combine(hash_a, hash_b)), ...);",
+               "(((hash_a = hash_combine(hash_a, hash_b))), ...);", Style);
   verifyFormat("return (0);", "return (((0)));", Style);
   verifyFormat("return (({ 0; }));", "return ((({ 0; })));", Style);
   verifyFormat("return ((... && std::is_convertible_v<TArgsLocal, TArgs>));",

[github-actions]

@owenca (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix. When you are done, please add the release:note label to this PR.