This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Added ReferenceAlignmentStyle option
Needs RevisionPublic

Authored by mrexodia on Apr 3 2017, 6:04 PM.
Tokens
"Like" token, awarded by catskul.

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

@MyDeveloperDay is this likely to land if the 3 items you mention are addressed? I'm trying to decide whether to adopt this patch to push it the last few cm over the finish line.

In my opinion this patch should not be merged. I honestly don’t believe
this is a legitimate way of formatting code, so why make it possible at all?

In my opinion this patch should not be merged. I honestly don’t believe
this is a legitimate way of formatting code, so why make it possible at all?

Mostly people want to migrate away from astyle without reviving formatting fights with their team will prevent adoption.

There are many of us stuck with no good options until we're able to convince our teams that clang-format will maintain their preference for whatever reason that preference exists.

MyDeveloperDay added a comment.EditedOct 14 2020, 12:01 PM

In my opinion this patch should not be merged. I honestly don’t believe
this is a legitimate way of formatting code, so why make it possible at all?

Mostly people want to migrate away from astyle without reviving formatting fights with their team will prevent adoption.

There are many of us stuck with no good options until we're able to convince our teams that clang-format will maintain their preference for whatever reason that preference exists.

I am keen to welcome people coming from astyle without there being an blockers or reigniting religious debates and we've had requests for this before.

I personally am not going to block such a change but @mrexodia didn't you write this patch? Did you change your mind? If so @catskul you'd need to either take ownership this patch or reimplement.

For me the biggest reason I've seen is that the gcc STL seems to be left align reference and right align pointer, I'm not 100% sure if this is deliberate, I wonder if this is why @STL_MSFT is keen, as MS stl is clang-formatted, but if we don't support this then comparing the MS stl to the gcc stl would be made harder.

This is enough for me to say, add it, allow people to choose, but don't change any of the defaults. I don't think this adds complexity.

(see one of the comment in D88227: [clang-format] Add a SpaceAroundPointerQualifiers style option)

For me the biggest reason I've seen is that the gcc STL seems to be left align reference and right align pointer, I'm not 100% sure if this is deliberate,
I wonder if this is why @STL_MSFT is keen, as MS stl is clang-formatted, but if we don't support this then comparing the MS stl to the gcc stl would be made harder.

No, it's just a stylistic preference on my part. (I avoid looking at GCC's libstdc++, but reference/pointer alignment is a raindrop in the vast ocean of differences between our implementations.)

My ideal preference is left-aligned references and middle-aligned pointers. This is because I think of the reference as inherently modifying the type, i.e. const int& as a cohesive unit, instead of the referenceness "belonging" to the variable. For pointers, given the choice between const int* ptr and const int *ptr, I prefer left-align for the same reason (ptr is a variable of type const int*, I never think of const int being the type of *ptr although that is historically how C syntax was viewed, as I understand it). That's what we've chosen for production code right now due to clang-format's limited configuration. However, because pointers exist as separate objects in memory, where const can be applied to both levels (unlike references), I most prefer middle-alignment: const int * const ptr (or int const * const ptr).

The limited configurability of clang-format is definitely an adoption blocker, and we had to live with several stylistic changes that weren't quite what we preferred, when we converted our codebase over. Given that opinions on the placement of references and pointers do vary, it seems reasonable to add such configuration to clang-format.

@catskul I think @STL_MSFT gives justification enough, I'm personally not of the belief there is one true format. I like clang-format to be flexible enough to meet the requirements that @STL_MSFT says, I also don't like @clang-format being the source of friction, and I don't like its constraints being the thing that prevents people from switching to clang-format.

I follow its mentions in twitter enough to know it can sometimes be a beast unto itself, and I've spend some of my time trying to remove those barriers. (as these were similar barriers from me adopting indent and astyle before it)

if @mrexodia doesn't want to pursue and they are ok with you taking this review over, then I certainly won't block it, but actively encourage it.

catskul added inline comments.Oct 26 2020, 9:39 PM
clang/unittests/Format/FormatTest.cpp
876

Is the request here to add the int Add2( BTree*& Root, char *szToAdd ) example? If not, can you clarify?

catskul added inline comments.Oct 26 2020, 9:43 PM
clang/lib/Format/Format.cpp
1450

not to make more work for myself, but should there be a separate DeriveReferenceAlignment?