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