[clang-format] Break an unwrapped line at a K&R C parameter decl

[owenpan]

Summary

Break an unwrapped line before the first parameter declaration in a K&R C function definition.

See PR51074.

[MyDeveloperDay]

Thanks for this, LGTM, maybe give the others some time

[curdeius]

Looks okay, but I was wondering if we don't want to guard all K&R-related changes behind e.g. `Standard: Cpp78``, so as not to possibly introduce strange bugs in newer modes.
It may be an overkill if there are 2 patches like this, but if there are more, that might become sensible to do so.
WDYT?

[MyDeveloperDay]

In D106112#2883301, @curdeius wrote:

Looks okay, but I was wondering if we don't want to guard all K&R-related changes behind e.g. `Standard: Cpp78``, so as not to possibly introduce strange bugs in newer modes.
It may be an overkill if there are 2 patches like this, but if there are more, that might become sensible to do so.
WDYT?

I wouldn't be opposed to that idea

[HazardyKnusperkeks]

In D106112#2883301, @curdeius wrote:

Looks okay, but I was wondering if we don't want to guard all K&R-related changes behind e.g. `Standard: Cpp78``, so as not to possibly introduce strange bugs in newer modes.
It may be an overkill if there are 2 patches like this, but if there are more, that might become sensible to do so.
WDYT?

I think this would be reasonable.

[owenpan]

In D106112#2883301, @curdeius wrote:

Looks okay, but I was wondering if we don't want to guard all K&R-related changes behind e.g. `Standard: Cpp78``, so as not to possibly introduce strange bugs in newer modes.
It may be an overkill if there are 2 patches like this, but if there are more, that might become sensible to do so.
WDYT?

I just reviewed the differences between K&R C (circa 1978) and ANSI/ISO C again and didn't see anything else that would impact clang-format, so a new Standard enum value for C78 is not needed. Nevertheless, we can add a boolean option e.g. C78ParameterDecl in the future if this patch causes regressions for some users. WDYT?

[curdeius]

In D106112#2885604, @owenpan wrote:

I just reviewed the differences between K&R C (circa 1978) and ANSI/ISO C again and didn't see anything else that would impact clang-format, so a new Standard enum value for C78 is not needed. Nevertheless, we can add a boolean option e.g. C78ParameterDecl in the future if this patch causes regressions for some users. WDYT?

Sounds reasonable.
Any way, you can go forward and land this patch.

clang/lib/Format/UnwrappedLineParser.h

private:
   void parsePPDirective();
   void parsePPDefine();
   void parsePPIf(bool IfDef);
   void parsePPElIf();
   void parsePPElse();
   void parsePPEndIf();
   void parsePPUnknown();
   void readTokenWithJavaScriptASI();
-  void parseStructuralElement();
+  void parseStructuralElement(bool IsTopLevel = false);
   bool tryToParseBracedList();
   bool parseBracedList(bool ContinueOnSemicolons = false, bool IsEnum = false,
                        tok::TokenKind ClosingBraceKind = tok::r_brace);
   void parseParens();
   void parseSquare(bool LambdaIntroducer = false);
   void parseIfThenElse();
   void parseTryCatch();
   void parseForOrWhileLoop();

clang/lib/Format/UnwrappedLineParser.cpp

do {
     case tok::l_square:
       if (Style.isCSharp()) {
         nextToken();
         parseCSharpAttribute();
         break;
       }
       LLVM_FALLTHROUGH;
     default:
-      parseStructuralElement();
+      parseStructuralElement(/*IsTopLevel=*/true);
       break;
     }
   } while (!eof());
 }
 
 void UnwrappedLineParser::calculateBraceTypes(bool ExpectClassBody) {
   // We'll parse forward through the tokens until we hit
   // a closing brace or eof - note that getNextToken() will
return FormatTok->isOneOf(
       tok::kw_throw, tok::kw_try, tok::kw_catch, Keywords.kw_finally,
       // declaration
       tok::kw_const, tok::kw_class, Keywords.kw_var, Keywords.kw_let,
       Keywords.kw_async, Keywords.kw_function,
       // import/export
       Keywords.kw_import, tok::kw_export);
 }
 
+// This function checks whether a token starts the first parameter declaration
+// in a K&R C (aka C78) function definition, e.g.:
+//   int f(a, b)
+//   short a, b;
+//   {
+//      return a + b;
+//   }
+static bool isC78ParameterDecl(const FormatToken *Tok) {
+  if (!Tok)
+    return false;
+
+  if (!Tok->isOneOf(tok::kw_int, tok::kw_char, tok::kw_float, tok::kw_double,
+                    tok::kw_struct, tok::kw_union, tok::kw_long, tok::kw_short,
+                    tok::kw_unsigned, tok::kw_register, tok::identifier))
+    return false;
+
+  Tok = Tok->Previous;
+  if (!Tok || Tok->isNot(tok::r_paren))
+    return false;
+
+  Tok = Tok->Previous;
+  if (!Tok || Tok->isNot(tok::identifier))
+    return false;
+
+  return Tok->Previous && Tok->Previous->isOneOf(tok::l_paren, tok::comma);
+}
+
 // readTokenWithJavaScriptASI reads the next token and terminates the current
 // line if JavaScript Automatic Semicolon Insertion must
 // happen between the current token and the next token.
 //
 // This method is conservative - it cannot cover all edge cases of JavaScript,
 // but only aims to correctly handle certain well known cases. It *must not*
 // return true in speculative cases.
 void UnwrappedLineParser::readTokenWithJavaScriptASI() {
if (NextMustBeValue && !NextEndsTemplateExpr && !PreviousStartsTemplateExpr &&
        Previous->isOneOf(tok::r_square, tok::r_paren, tok::plusplus,
                          tok::minusminus)))
     return addUnwrappedLine();
   if ((PreviousMustBeValue || Previous->is(tok::r_paren)) &&
       isJSDeclOrStmt(Keywords, Next))
     return addUnwrappedLine();
 }
 
-void UnwrappedLineParser::parseStructuralElement() {
+void UnwrappedLineParser::parseStructuralElement(bool IsTopLevel) {
   assert(!FormatTok->is(tok::l_brace));
   if (Style.Language == FormatStyle::LK_TableGen &&
       FormatTok->is(tok::pp_include)) {
     nextToken();
     if (FormatTok->is(tok::string_literal))
       nextToken();
     addUnwrappedLine();
     return;
case tok::semi:
       nextToken();
       addUnwrappedLine();
       return;
     case tok::r_brace:
       addUnwrappedLine();
       return;
     case tok::l_paren:
       parseParens();
+      // Break the unwrapped line if a K&R C function definition has a parameter
+      // declaration.
+      if (!IsTopLevel || !Style.isCpp())
+        break;
+      if (!Previous || Previous->isNot(tok::identifier))
+        break;
+      if (Previous->Previous && Previous->Previous->is(tok::at))
+        break;
+      if (isC78ParameterDecl(FormatTok)) {
+        addUnwrappedLine();
+        return;
+      }
       break;
     case tok::kw_operator:
       nextToken();
       if (FormatTok->isBinaryOperator())
         nextToken();
       break;
     case tok::caret:
       nextToken();

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, ReturnTypeBreakingStyle) {
 
   Style.BreakBeforeBraces = FormatStyle::BS_Custom;
   Style.BraceWrapping.AfterFunction = true;
   verifyFormat("int f(i);\n" // No break here.
                "int\n"       // Break here.
                "f(i)\n"
                "{\n"
                "  return i + 1;\n"
-               "}\n",
+               "}",
+               Style);
+  verifyFormat("int f(a, b, c);\n" // No break here.
+               "int\n"             // Break here.
+               "f(a, b, c)\n"      // Break here.
+               "short a, b;\n"
+               "float c;\n"
+               "{\n"
+               "  return a + b < c;\n"
+               "}",
                Style);
 
   Style = getGNUStyle();
 
   // Test for comments at the end of function declarations.
   verifyFormat("void\n"
                "foo (int a, /*abc*/ int b) // def\n"
                "{\n"
TEST_F(FormatTest, UnderstandsUsesOfStarAndAmp) {
   TypeMacros.TypenameMacros = {"LIST"};
   verifyFormat("vector<LIST(uint64_t)> v;", TypeMacros);
   verifyFormat("vector<LIST(uint64_t) *> v;", TypeMacros);
   verifyFormat("vector<LIST(uint64_t) **> v;", TypeMacros);
   verifyFormat("vector<LIST(uint64_t) *attr> v;", TypeMacros);
   verifyFormat("vector<A(uint64_t) * attr> v;", TypeMacros); // multiplication
 
   FormatStyle CustomQualifier = getLLVMStyle();
-  // Add indentifers that should not be parsed as a qualifier by default.
+  // Add identifiers that should not be parsed as a qualifier by default.
   CustomQualifier.AttributeMacros.push_back("__my_qualifier");
   CustomQualifier.AttributeMacros.push_back("_My_qualifier");
   CustomQualifier.AttributeMacros.push_back("my_other_qualifier");
   verifyFormat("vector<a * __my_qualifier> parse_as_multiply;");
   verifyFormat("vector<a *__my_qualifier> v;", CustomQualifier);
   verifyFormat("vector<a * _My_qualifier> parse_as_multiply;");
   verifyFormat("vector<a *_My_qualifier> v;", CustomQualifier);
   verifyFormat("vector<a * my_other_qualifier> parse_as_multiply;");