[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 aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa GUARDED_BY(aaaaaaaaaaaa) =\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