Page MenuHomePhabricator

[clang-format] Improve heuristic for detecting function declarations
Needs ReviewPublic

Authored by arichardson on Sep 2 2020, 7:17 AM.

Details

Summary

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)

Diff Detail

Event Timeline

arichardson created this revision.Sep 2 2020, 7:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 2 2020, 7:17 AM
arichardson requested review of this revision.Sep 2 2020, 7:17 AM

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

clang/lib/Format/TokenAnnotator.cpp
2427

how hard would it be to do the TODO?

clang/unittests/Format/FormatTest.cpp
5507

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)

6715

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

6730

break it here

6740

break it here.

6762

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?

arichardson added inline comments.Sep 7 2020, 1:52 AM
clang/lib/Format/TokenAnnotator.cpp
2427

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.

clang/unittests/Format/FormatTest.cpp
5507

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.

6762

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.