[PATCH] D87028: [clang-format] Improve heuristic for detecting function declarations

[Alexander Richardson via Phabricator via cfe-commits]

arichardson added inline comments.



Comment at: clang/lib/Format/TokenAnnotator.cpp:2427
+  // inside a function this should always be treated as a variable.
+  return CouldBeTypeList && Line.Level == 0;
 }

MyDeveloperDay wrote:
> how hard would it be to do the TODO?
I am not that familiar with the clang-format codebase yet, but it appears to me 
that this information is not tracked at all. It should be quite easy to add 
another `Scope` (or similar) member that is an enum for 
global/function/namespace/class/other and then update that in all cases that 
also change the level member.



Comment at: clang/unittests/Format/FormatTest.cpp:5507
+  Google);
   verifyGoogleFormat(
   "bool aa GUARDED_BY() =\n"

MyDeveloperDay wrote:
> I want to approve this change, but I HATE changing unit tests (Beyonce rule), 
> I'm struggling to see if we are changing anything here? or if you are just 
> qualifying it a little better because the usage is different depending on 
> where its used  (as a function,as a variable)
> 
Before a non-empty `()` with a single indentifier inside was always treated as 
a variable, with this change it's (in this case incorrectly) parsed as function.



Comment at: clang/unittests/Format/FormatTest.cpp:6762
+   "} // namespace namspace_scope\n",
+   Style);
   verifyFormat("class E {\n"

MyDeveloperDay wrote:
> Nit that is a mother of an assert, but when it fails.. how easy is it going 
> to be to debug where it goes wrong exactly.
> 
> Could we break it up a little?
`I agree this is too long. I was just trying to avoid repeating the `namespace 
{}/void fn()` prefix. Will try to break it up for next version of this patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87028/new/

https://reviews.llvm.org/D87028

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D87028: [clang-format] Improve heuristic for detecting function declarations

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

Thanks for the patch, I think this looks like a comprehensive improvement, a 
new nits only




Comment at: clang/lib/Format/TokenAnnotator.cpp:2427
+  // inside a function this should always be treated as a variable.
+  return CouldBeTypeList && Line.Level == 0;
 }

how hard would it be to do the TODO?



Comment at: clang/unittests/Format/FormatTest.cpp:5507
+  Google);
   verifyGoogleFormat(
   "bool aa GUARDED_BY() =\n"

I want to approve this change, but I HATE changing unit tests (Beyonce rule), 
I'm struggling to see if we are changing anything here? or if you are just 
qualifying it a little better because the usage is different depending on where 
its used  (as a function,as a variable)




Comment at: clang/unittests/Format/FormatTest.cpp:6715
+   "funcdecl(SomeType param1, OtherType param2);\n"
+   // Also handle parameter lists declaration without names (but
+   // only at the top level, not inside functions

if you have to put a comment in the test then you probably should have broken 
the verifyFormat



Comment at: clang/unittests/Format/FormatTest.cpp:6730
+   "SomeType x = var * funcdecl(var, otherVar);\n"
+   "void\n"
+   "function_scope() {\n"

break it here



Comment at: clang/unittests/Format/FormatTest.cpp:6740
+   "  SomeType *funcdecl(SomeType, OtherType);\n"
+   "}\n"
+   "namespace namspace_scope {\n"

break it here.



Comment at: clang/unittests/Format/FormatTest.cpp:6762
+   "} // namespace namspace_scope\n",
+   Style);
   verifyFormat("class E {\n"

Nit that is a mother of an assert, but when it fails.. how easy is it going to 
be to debug where it goes wrong exactly.

Could we break it up a little?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87028/new/

https://reviews.llvm.org/D87028

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D87028: [clang-format] Improve heuristic for detecting function declarations

[Alexander Richardson via Phabricator via cfe-commits]

arichardson updated this revision to Diff 289508.
arichardson added a comment.

fix tests


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87028/new/

https://reviews.llvm.org/D87028

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
@@ -5482,12 +5482,28 @@
   "   a));");
   verifyFormat("bool aaa\n"
"__attribute__((unused));");
-  verifyGoogleFormat(
+  FormatStyle Google = getGoogleStyle();
+  verifyFormat(
   "bool a\n"
-  "GUARDED_BY();");
-  verifyGoogleFormat(
+  "GUARDED_BY();\n" // parsed as function decl
+  "void f() {\n"
+  "  bool aaa\n"
+  "  GUARDED_BY();\n" // parsed as variable decl
+  "}\n"
+  "class Cls {\n"
+  "  bool a\n"
+  "  GUARDED_BY();\n" // parsed as variable decl
+  "};",
+  Google);
+  verifyFormat(
   "bool a\n"
-  "GUARDED_BY();");
+  "GUARDED_BY();", // parsed as function decl
+  Google);
+  Google.AttributeMacros = {"GUARDED_BY"};
+  verifyFormat(
+  "bool a GUARDED_BY(\n"
+  ");", // parsed as variable decl
+  Google);
   verifyGoogleFormat(
   "bool aa GUARDED_BY() =\n"
   "::aaa;");
@@ -6683,10 +6699,67 @@
   // All declarations and definitions should have the return type moved to its
   // own line.
   Style.AlwaysBreakAfterReturnType = FormatStyle::RTBS_All;
+  Style.NamespaceIndentation = FormatStyle::NI_All;
   Style.TypenameMacros = {"LIST"};
   verifyFormat("SomeType\n"
"funcdecl(LIST(uint64_t));",
Style);
+  verifyFormat("SomeType\n"
+   "funcdecl();\n"
+   "SomeType\n"
+   "funcdecl(SomeType paramname);\n"
+   "SomeType\n"
+   "funcdecl(_Atomic(uint64_t));\n"
+   "SomeType\n"
+   "funcdecl(SomeType param1, OtherType param2);\n"
+   // Also handle parameter lists declaration without names (but
+   // only at the top level, not inside functions
+   "SomeType\n"
+   "funcdecl(SomeType);\n"
+   "SomeType\n"
+   "funcdecl(SomeType, OtherType);\n"
+   // Check that any kind of expression/operator results in parsing
+   // it as a variable declaration and not a function
+   "SomeType vardecl(var + otherVar);\n"
+   "SomeType vardecl(func());\n"
+   "SomeType vardecl(someFunc(arg));\n"
+   "SomeType vardecl(var, var - otherVar);\n"
+   "SomeType x = var * funcdecl();\n"
+   "SomeType x = var * funcdecl(otherVar);\n"
+   "SomeType x = var * funcdecl(var, otherVar);\n"
+   "void\n"
+   "function_scope() {\n"
+   "  SomeType x = var * funcdecl();\n"
+   "  SomeType x = var * funcdecl(otherVar);\n"
+   "  SomeType x = var * funcdecl(var, otherVar);\n"
+   // While clang will parse these as function declarations, at
+   // least for now clang-format assumes they are variables.
+   "  SomeType *funcdecl();\n"
+   "  SomeType *funcdecl(SomeType);\n"
+   "  SomeType *funcdecl(SomeType, OtherType);\n"
+   "}\n"
+   "namespace namspace_scope {\n"
+   // TODO: Should we also parse these as a function declaration
+   //  and not as a variable inside namespaces?
+   "  SomeType\n"
+   "  funcdecl();\n"
+   "  SomeType\n"
+   "  funcdecl(SomeType paramname);\n"
+   "  SomeType\n"
+   "  funcdecl(_Atomic(uint64_t));\n"
+   "  SomeType\n"
+   "  funcdecl(SomeType param1, OtherType param2);\n"
+   "  SomeType decl(SomeType);\n"
+   "  SomeType decl(SomeType, OtherType);\n"
+   "  SomeType vardecl(var + otherVar);\n"
+   "  SomeType vardecl(func());\n"
+   "  SomeType vardecl(someFunc(arg));\n"
+   "  SomeType vardecl(var, var - otherVar);\n"
+   "  SomeType x = var * funcdecl();\n"
+   "  SomeType x = 

[PATCH] D87028: [clang-format] Improve heuristic for detecting function declarations

[Alexander Richardson via Phabricator via cfe-commits]

arichardson created this revision.
arichardson added reviewers: MyDeveloperDay, JakeMerdichAMD, sammccall, 
curdeius.
Herald added subscribers: cfe-commits, jfb.
Herald added a project: clang.
arichardson requested review of this revision.

This change also comma-separated identifiers inside parentheses as a
possible function declaration.

When using the always break after return type setting:
Before:
SomeType funcdecl(SomeType);
SomeType funcdecl(SomeType, OtherType);

After:
SomeType
funcdecl(SomeType);
SomeType
funcdecl(SomeType, OtherType);

Depends on D87007  (to apply cleanly)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D87028

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
@@ -6683,10 +6683,69 @@
   // All declarations and definitions should have the return type moved to its
   // own line.
   Style.AlwaysBreakAfterReturnType = FormatStyle::RTBS_All;
+  Style.NamespaceIndentation = FormatStyle::NI_All;
   Style.TypenameMacros = {"LIST"};
   verifyFormat("SomeType\n"
"funcdecl(LIST(uint64_t));",
Style);
+  verifyFormat("SomeType\n"
+   "funcdecl();\n"
+   "SomeType\n"
+   "funcdecl(SomeType paramname);\n"
+   "SomeType\n"
+   "funcdecl(_Atomic(uint64_t));\n"
+   "SomeType\n"
+   "funcdecl(SomeType param1, OtherType param2);\n"
+   // Also handle parameter lists declaration without names (but
+   // only at the top level, not inside functions
+   "SomeType\n"
+   "funcdecl(SomeType);\n"
+   "SomeType\n"
+   "funcdecl(SomeType, OtherType);\n"
+   // Check that any kind of expression/operator results in parsing
+   // it as a variable declaration and not a function
+   "SomeType vardecl(var + otherVar);\n"
+   "SomeType vardecl(func());\n"
+   "SomeType vardecl(someFunc(arg));\n"
+   "SomeType vardecl(var, var - otherVar);\n"
+   "SomeType x = var * funcdecl();\n"
+   "SomeType x = var * funcdecl(otherVar);\n"
+   "SomeType x = var * funcdecl(var, otherVar);\n"
+   "void\n"
+   "function_scope() {\n"
+   "  SomeType x = var * funcdecl();\n"
+   "  SomeType x = var * funcdecl(otherVar);\n"
+   "  SomeType x = var * funcdecl(var, otherVar);\n"
+   // While clang will parse these as function declarations, at
+   // least for now clang-format assumes they are variables.
+   "  SomeType *funcdecl();\n"
+   "  SomeType *funcdecl(SomeType);\n"
+   "  SomeType *funcdecl(SomeType, OtherType);\n"
+   "}\n"
+   "namespace namspace_scope {\n"
+   "  SomeType\n"
+   "  funcdecl();\n"
+   "  SomeType\n"
+   "  funcdecl(SomeType paramname);\n"
+   "  SomeType\n"
+   "  funcdecl(_Atomic(uint64_t));\n"
+   "  SomeType\n"
+   "  funcdecl(SomeType param1, OtherType param2);\n"
+   // Inside a namespace we should also parse these as a function
+   // declaration and not as a variable.
+   "  SomeType\n"
+   "  funcdecl(SomeType);\n"
+   "  SomeType\n"
+   "  funcdecl(SomeType, OtherType);\n"
+   "  SomeType vardecl(var + otherVar);\n"
+   "  SomeType vardecl(func());\n"
+   "  SomeType vardecl(someFunc(arg));\n"
+   "  SomeType vardecl(var, var - otherVar);\n"
+   "  SomeType x = var * funcdecl();\n"
+   "  SomeType x = var * funcdecl(otherVar);\n"
+   "  SomeType x = var * funcdecl(var, otherVar);\n"
+   "} // namespace namspace_scope\n",
+   Style);
   verifyFormat("class E {\n"
"  int\n"
"  f() {\n"
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -2398,12 +2398,17 @@
   if (Next->MatchingParen->Next &&
   Next->MatchingParen->Next->is(TT_PointerOrReference))
 return true;
+  // Treat  cases where the parameter list only contains comma-separated
+  // identifiers as function declarations. For example:
+  // `SomeType funcdecl(OtherType)` or `SomeType funcdecl(Type1, Type2)`
+  bool CouldBeTypeList = true;
   for (const FormatToken *Tok = Next->Next; Tok && Tok != Next->MatchingParen;
Tok = Tok->Next) {
 if