Page MenuHomePhabricator

[Clang-Format] Add ReferenceAlignment directive
AcceptedPublic

Authored by skirkovski on Fri, Jun 11, 12:45 AM.

Details

Summary

This is a roll-forward of D90238 originally implemented by @mrexodia and
@catskul

This introduces ReferenceAlignment style option modeled around
PointerAlignment.
Style implementors can specify Left, Right, Middle or Pointer to
follow whatever the PointerAlignment option specifies.

Changes since D90238:

  • Added tests for ref-qualified functions
  • Reorganized the newly introduced TEST_F so pure LLVM style tests come first
  • Removed ReferenceAlignment assignements in LLVM derived styles (now only the LLVM style specifies RAS_Pointer)
  • Split the getTokenPointerAlignment helper in getTokenReferenceAlignment and getTokenPointerOrReferenceAlignment
  • Test clang-format by running unittests and reformatting the entire compiler-rt tree

Diff Detail

Event Timeline

skirkovski requested review of this revision.Fri, Jun 11, 12:45 AM
skirkovski created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFri, Jun 11, 12:45 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

A new nits but I'd say it was pretty good.

clang/lib/Format/Format.cpp
1368

if we are inheriting from LLVM we only need this in the LLVM base tyle

clang/unittests/Format/FormatTest.cpp
1243

could you put each line on its own line. makes it easier to reason about

1250

ditto

1256

ditto

Can you also add tests with the alignment of declarations? We already have such for pointers.

clang/include/clang/Format/Format.h
2735

Please move below the RawStringFormats, same for the comparison etc.

clang/unittests/Format/FormatTest.cpp
1243–1244

Please also add the empty line.

Remove PAS_Pointer from Webkit style (since it derives from LLVM which already has it)
Add empty lines between style change in the test case
Split a verifyFormat with 3 different cases into different ones
Add test cases for declaration alignemnts

MyDeveloperDay accepted this revision.Sat, Jun 12, 2:58 AM

LGTM

clang/include/clang/Format/Format.h
2676

nice catch!

clang/lib/Format/Format.cpp
689

Nit: so normally these would be alphabetic, totally understand why you would put them together but I think we should probably follow convention and put Reference type after RawStringFormats

This revision is now accepted and ready to land.Sat, Jun 12, 2:58 AM
skirkovski marked 3 inline comments as done.

Sort ReferenceAlignment after RawStringFormats in mapping()

@MyDeveloperDay,

Thanks for the review. I do not have submit access. Can you take care of landing the revision when you have some free time ?

clang/unittests/Format/FormatTest.cpp
1243

Transformed each assignment to a separate verifyFormat. Is this what you meant ?

Added test case for consecutive alignments which covers the previous verifyFormat with multiple assignemnts.

Thanks for the work!

Ping,

I do not have commit access, can someone submit this as time permits ?

Ping,

I do not have commit access, can someone submit this as time permits ?

Please state the name and email for the commit, I will commit it on the weekend.

This patch needs rebasing, please also make comments as "Done" once you have addressed them.

clang/lib/Format/TokenAnnotator.cpp
4263

you are missing a return value here

C:\cygwin64\buildareas\clang\llvm-project\clang\lib\Format\TokenAnnotator.cpp(4264) : warning C4715: 'clang::format::TokenAnnotator::getTokenReferenceAlignment': not all control paths return a value

Rebase and fix warning

skirkovski marked 5 inline comments as done.Thu, Jun 17, 9:20 AM

Rebased and fixed warning. My local compiler was yelling at me for adding a default case to switch which already covers everything.

For the commit:

Author: Seraphime Kirkovski
email: skirkovski at vmware.com