Page MenuHomePhabricator

[clang-format] Added ReferenceAlignmentStyle option
Needs RevisionPublic

Authored by mrexodia on Apr 3 2017, 6:04 PM.

Details

Summary

This adds an option to control reference alignment separately from pointer alignment.

.clang-format:

PointerAlignment: Left
ReferenceAlignment: Middle

Before:

int*myFunction1(int*ptr1,int&ref1);
int&myFunction2(int&ref2,int*ptr2);
int&&myFunction3(int&&ref3,int**ptr3);

After:

int* myFunction1(int* ptr1, int & ref1);
int & myFunction2(int & ref2, int* ptr2);
int && myFunction3(int && ref3, int** ptr3);

Diff Detail

Event Timeline

mrexodia created this revision.Apr 3 2017, 6:04 PM
mrexodia edited the summary of this revision. (Show Details)Apr 3 2017, 6:09 PM

I'd love to have this feature; is there any chance of this year-old patch being accepted?

Herald added a project: Restricted Project. · View Herald TranscriptFeb 20 2019, 9:58 AM

This revision needs unit tests in clang/unittests/Format, you should also add changes to the docs/ReleaseNotes.rst and docs/ClangFormatStyleOptions.rst

Please add unit tests to show this working, otherwise later someone will come along and break the behavior

Can you provide some evidence:

  • that this is in a style guide that's used widely enough
  • why you believe this matters
MyDeveloperDay edited projects, added Restricted Project; removed Restricted Project.Sep 25 2019, 1:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 25 2019, 1:51 AM
mrexodia updated this revision to Diff 223660.Oct 7 2019, 2:33 PM
mrexodia edited the summary of this revision. (Show Details)
mrexodia added a reviewer: MyDeveloperDay.

@MyDeveloperDay

I rebased and added unit tests + and ran clang-format.

@klimek

I just need this to be able to replace AStyle for my open source project (x64dbg). I just rebased because @STL_MSFT asked for it and I had some free time.

I don't think it is actually possible to be consistent with separate options for pointers and references. See for example https://docs.microsoft.com/en-us/cpp/cpp/references-to-pointers?view=vs-2019 which has:

int Add2( BTree*& Root, char *szToAdd )

Properly supporting this formatting likely isn't going to be pretty, but anyone is free to try with this rebased patch.

I hadn't realized that AStyle had a separate reference alignment style, anyone coming from Astyle potentially would miss that behaviour as it would represent itself as subtle changes.

For this example you state, with your patch

int Add2( BTree*& Root, char *szToAdd )

How is this formatted in comparison to Astyle?

MyDeveloperDay added a comment.EditedOct 8 2019, 12:23 AM

Thank you for the patch, just a couple of minor things (the doc is important).

As Astyle has this option I think this is a gap in clang-format (this is also not the first request I've seen for this https://bugs.llvm.org/show_bug.cgi?id=42165)

Given your project is waiting to use it (https://github.com/x64dbg/x64dbg) and the number of forks/stars I think we should consider this a significant enough project using this style to warrant it being worth landing.

clang/include/clang/Format/Format.h
1681

You are missing a documentation change (there is a python script which update the ClangFormatStyleOption.rst from this header)

clang/unittests/Format/FormatTest.cpp
857

Nit: for completeness (I might be tempted to add the other styles PAS_Middle and PAS_Right for pointer alignment where ReferenceAlignment is RAS_Pointer (given there is code which checks)

getTokenPointerAlignment(Left) != FormatStyle::PAS_Right
876

add your example as a test just comment it out and add FIXME if it doesn't work, then it will tell people this is what we are aiming for.

MyDeveloperDay requested changes to this revision.Oct 8 2019, 12:23 AM
This revision now requires changes to proceed.Oct 8 2019, 12:23 AM