[clang-format] improve distinction of K&R function definitions vs attributes

[krasimir]

Summary

After
https://github.com/llvm/llvm-project/commit/9da70ab3d43c79116f80fc06aa7cf517374ce42c
we saw a few regressions around trailing attribute definitions and in
typedefs (examples in the added test cases). There's some tension
distinguishing K&R definitions from attributes at the parser level,
where we have to decide if we need to put the type of the K&R definition
on a new unwrapped line before we have access to the rest of the line,
so we're scanning backwards and looking for a pattern like f(a, b). But
this type of pattern could also be an attribute macro, or the whole
declaration could be a typedef itself. I updated the code to check for a
typedef at the beginning of the line and to not consider raw identifiers
as possible first K&R declaration (but treated as an attribute macro
instead). This is not 100% correct heuristic, but I think it should be
reasonably good in practice, where we'll:

• likely be in some very C-ish code when using K&R style (e.g., stuff that uses struct name a; instead of name a;
• likely be in some very C++-ish code when using attributes
• unlikely mix up the two in the same declaration.

Ideally, we should only decide to add the unwrapped line before the K&R
declaration after we've scanned the rest of the line an noticed the
variable declarations and the semicolon, but the way the parser is
organized I don't see a good way to do this in the current parser, which
only has good context for the previously visited tokens. I also tried
not emitting an unwrapped line there and trying to resolve the situation
later in the token annotator and the continuation indenter, and that
approach seems promising, but I couldn't make it to work without
messing up a bunch of other cases in unit tests.

[MyDeveloperDay]

Thanks for doing this, it LGTM. I personally think tok::identifier tends to be just too general, its hard to use it correctly in the rules especially in the presence of macros.

[owenpan]

In D107950#2941015, @MyDeveloperDay wrote:

I personally think tok::identifier tends to be just too general

However, you can't avoid it as it's used for user-defined types:

typedef unsigned char byte;

byte *f(a)
byte a[];
{
  return a && *a ? a + 1 : 0;
}

I will try to come up with a solution.

clang/lib/Format/UnwrappedLineParser.cpp

 //===--- UnwrappedLineParser.cpp - Format C++ code ------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 ///
 /// \file
 /// This file contains the implementation of the UnwrappedLineParser,
 /// which turns a stream of tokens into UnwrappedLines.
 ///
 //===----------------------------------------------------------------------===//
 
 #include "UnwrappedLineParser.h"
 #include "FormatToken.h"
+#include "clang/Basic/TokenKinds.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/raw_ostream.h"
 
 #include <algorithm>
 
 #define DEBUG_TYPE "format-parser"
 

 //      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))
+                    tok::kw_unsigned, tok::kw_register))
     return false;
 
   Tok = Tok->Previous;
   if (!Tok || Tok->isNot(tok::r_paren))
     return false;
 
   Tok = Tok->Previous;
   if (!Tok || Tok->isNot(tok::identifier))
case tok::l_paren:
       // 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)) {
+      if (!Line->Tokens.begin()->Tok->is(tok::kw_typedef) &&
+          isC78ParameterDecl(FormatTok)) {
         addUnwrappedLine();
         return;
       }
       break;
     case tok::kw_operator:
       nextToken();
       if (FormatTok->isBinaryOperator())
         nextToken();

clang/unittests/Format/FormatTest.cpp

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);
-  // The return breaking style doesn't affect object definitions with
-  // attribute-like macros.
+
+  // The return breaking style doesn't affect:
+  // * function and object definitions with attribute-like macros
   verifyFormat("Tttttttttttttttttttttttt ppppppppppppppp\n"
                "    ABSL_GUARDED_BY(mutex) = {};",
                getGoogleStyleWithColumns(40));
   verifyFormat("Tttttttttttttttttttttttt ppppppppppppppp\n"
                "    ABSL_GUARDED_BY(mutex);  // comment",
                getGoogleStyleWithColumns(40));
+  verifyFormat("Tttttttttttttttttttttttt ppppppppppppppp\n"
+               "    ABSL_GUARDED_BY(mutex1)\n"
+               "        ABSL_GUARDED_BY(mutex2);",
+               getGoogleStyleWithColumns(40));
+  verifyFormat("Tttttt f(int a, int b)\n"
+               "    ABSL_GUARDED_BY(mutex1)\n"
+               "        ABSL_GUARDED_BY(mutex2);",
+               getGoogleStyleWithColumns(40));
+  // * typedefs
+  verifyFormat("typedef ATTR(X) char x;", getGoogleStyle());
 
   Style = getGNUStyle();
 
   // Test for comments at the end of function declarations.
   verifyFormat("void\n"
                "foo (int a, /*abc*/ int b) // def\n"
                "{\n"
                "}\n",