[clang-format] Added ReferenceAlignmentStyle option

[mrexodia]

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);

[STL_MSFT]

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

[MyDeveloperDay]

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

[MyDeveloperDay]

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

[klimek]

Can you provide some evidence:

• that this is in a style guide that's used widely enough
• why you believe this matters

[mrexodia]

@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.

[MyDeveloperDay]

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]

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 ↗

(On Diff #223660)

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 ↗

(On Diff #223660)

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 ↗

(On Diff #223660)

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.

[catskul]

@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.

[mrexodia]

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?

[catskul]

In D31635#2329449, @mrexodia wrote:

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]

In D31635#2330075, @catskul wrote:

In D31635#2329449, @mrexodia wrote:

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)

[STL_MSFT]

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.

[MyDeveloperDay]

@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]

clang/unittests/Format/FormatTest.cpp

876 ↗

(On Diff #223660)

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

[catskul]

clang/lib/Format/Format.cpp

1450 ↗

(On Diff #223660)

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

[MyDeveloperDay]

@mrexodia can we Abandoned this now due to D104096: [Clang-Format] Add ReferenceAlignment directive

include/clang/Format/Format.h

 
   /// \brief The penalty for each character outside of the column limit.
   unsigned PenaltyExcessCharacter;
 
   /// \brief Penalty for putting the return type of a function onto its own
   /// line.
   unsigned PenaltyReturnTypeOnItsOwnLine;
 
-  /// \brief The ``&`` and ``*`` alignment style.
+  /// \brief The ``&``, ``&&`` and ``*`` alignment style.
   enum PointerAlignmentStyle {
     /// Align pointer to the left.
     /// \code
     ///   int* a;
     /// \endcode
     PAS_Left,
     /// Align pointer to the right.
     /// \code
     ///   int *a;
     /// \endcode
     PAS_Right,
     /// Align pointer in the middle.
     /// \code
     ///   int * a;
     /// \endcode
     PAS_Middle
   };
 
   /// \brief Pointer and reference alignment style.
   PointerAlignmentStyle PointerAlignment;
 
+  /// \brief The ``&`` and ``&&`` alignment style.
+  enum ReferenceAlignmentStyle {
+      /// Align reference like ``PointerAlignment``.
+      RAS_Pointer,
+      /// Align reference to the left.
+      /// \code
+      ///   int& a;
+      /// \endcode
+      RAS_Left,
+      /// Align reference to the right.
+      /// \code
+      ///   int &a;
+      /// \endcode
+      RAS_Right,
+      /// Align reference in the middle.
+      /// \code
+      ///   int & a;
+      /// \endcode
+      RAS_Middle
+  };
+
+  /// \brief Reference alignment style (overrides ``PointerAlignment`` for references).
+  ReferenceAlignmentStyle ReferenceAlignment;
+
   /// \brief If ``true``, clang-format will attempt to re-flow comments.
   /// \code
   ///    false:
   ///    // veryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongComment with plenty of information
   ///    /* second veryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongComment with plenty of information */
   ///
   ///    true:
   ///    // veryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongComment with plenty of
return AccessModifierOffset == R.AccessModifierOffset &&
            PenaltyBreakBeforeFirstCallParameter ==
                R.PenaltyBreakBeforeFirstCallParameter &&
            PenaltyBreakComment == R.PenaltyBreakComment &&
            PenaltyBreakFirstLessLess == R.PenaltyBreakFirstLessLess &&
            PenaltyBreakString == R.PenaltyBreakString &&
            PenaltyExcessCharacter == R.PenaltyExcessCharacter &&
            PenaltyReturnTypeOnItsOwnLine == R.PenaltyReturnTypeOnItsOwnLine &&
            PointerAlignment == R.PointerAlignment &&
+           ReferenceAlignment == R.ReferenceAlignment &&
            SpaceAfterCStyleCast == R.SpaceAfterCStyleCast &&
            SpaceAfterTemplateKeyword == R.SpaceAfterTemplateKeyword &&
            SpaceBeforeAssignmentOperators == R.SpaceBeforeAssignmentOperators &&
            SpaceBeforeParens == R.SpaceBeforeParens &&
            SpaceInEmptyParentheses == R.SpaceInEmptyParentheses &&
            SpacesBeforeTrailingComments == R.SpacesBeforeTrailingComments &&
            SpacesInAngles == R.SpacesInAngles &&
            SpacesInContainerLiterals == R.SpacesInContainerLiterals &&

lib/Format/Format.cpp

static void enumeration(IO &IO, FormatStyle::PointerAlignmentStyle &Value) {
     IO.enumCase(Value, "Right", FormatStyle::PAS_Right);
 
     // For backward compatibility.
     IO.enumCase(Value, "true", FormatStyle::PAS_Left);
     IO.enumCase(Value, "false", FormatStyle::PAS_Right);
   }
 };
 
+template <> struct ScalarEnumerationTraits<FormatStyle::ReferenceAlignmentStyle> {
+  static void enumeration(IO &IO, FormatStyle::ReferenceAlignmentStyle &Value) {
+    IO.enumCase(Value, "Pointer", FormatStyle::RAS_Pointer);
+    IO.enumCase(Value, "Middle", FormatStyle::RAS_Middle);
+    IO.enumCase(Value, "Left", FormatStyle::RAS_Left);
+    IO.enumCase(Value, "Right", FormatStyle::RAS_Right);
+  }
+};
+
 template <>
 struct ScalarEnumerationTraits<FormatStyle::SpaceBeforeParensOptions> {
   static void enumeration(IO &IO,
                           FormatStyle::SpaceBeforeParensOptions &Value) {
     IO.enumCase(Value, "Never", FormatStyle::SBPO_Never);
     IO.enumCase(Value, "ControlStatements",
                 FormatStyle::SBPO_ControlStatements);
     IO.enumCase(Value, "Always", FormatStyle::SBPO_Always);
static void mapping(IO &IO, FormatStyle &Style) {
     IO.mapOptional("PenaltyBreakComment", Style.PenaltyBreakComment);
     IO.mapOptional("PenaltyBreakFirstLessLess",
                    Style.PenaltyBreakFirstLessLess);
     IO.mapOptional("PenaltyBreakString", Style.PenaltyBreakString);
     IO.mapOptional("PenaltyExcessCharacter", Style.PenaltyExcessCharacter);
     IO.mapOptional("PenaltyReturnTypeOnItsOwnLine",
                    Style.PenaltyReturnTypeOnItsOwnLine);
     IO.mapOptional("PointerAlignment", Style.PointerAlignment);
+    IO.mapOptional("ReferenceAlignment", Style.ReferenceAlignment);
     IO.mapOptional("ReflowComments", Style.ReflowComments);
     IO.mapOptional("SortIncludes", Style.SortIncludes);
     IO.mapOptional("SpaceAfterCStyleCast", Style.SpaceAfterCStyleCast);
     IO.mapOptional("SpaceAfterTemplateKeyword", Style.SpaceAfterTemplateKeyword);
     IO.mapOptional("SpaceBeforeAssignmentOperators",
                    Style.SpaceBeforeAssignmentOperators);
     IO.mapOptional("SpaceBeforeParens", Style.SpaceBeforeParens);
     IO.mapOptional("SpaceInEmptyParentheses", Style.SpaceInEmptyParentheses);
FormatStyle getLLVMStyle() {
   LLVMStyle.TabWidth = 8;
   LLVMStyle.MaxEmptyLinesToKeep = 1;
   LLVMStyle.KeepEmptyLinesAtTheStartOfBlocks = true;
   LLVMStyle.NamespaceIndentation = FormatStyle::NI_None;
   LLVMStyle.ObjCBlockIndentWidth = 2;
   LLVMStyle.ObjCSpaceAfterProperty = false;
   LLVMStyle.ObjCSpaceBeforeProtocolList = true;
   LLVMStyle.PointerAlignment = FormatStyle::PAS_Right;
+  LLVMStyle.ReferenceAlignment = FormatStyle::RAS_Pointer;
   LLVMStyle.SpacesBeforeTrailingComments = 1;
   LLVMStyle.Standard = FormatStyle::LS_Cpp11;
   LLVMStyle.UseTab = FormatStyle::UT_Never;
   LLVMStyle.ReflowComments = true;
   LLVMStyle.SpacesInParentheses = false;
   LLVMStyle.SpacesInSquareBrackets = false;
   LLVMStyle.SpaceInEmptyParentheses = false;
   LLVMStyle.SpacesInContainerLiterals = true;
FormatStyle getGoogleStyle(FormatStyle::LanguageKind Language) {
   GoogleStyle.DerivePointerAlignment = true;
   GoogleStyle.IncludeCategories = {{"^<.*\\.h>", 1}, {"^<.*", 2}, {".*", 3}};
   GoogleStyle.IncludeIsMainRegex = "([-_](test|unittest))?$";
   GoogleStyle.IndentCaseLabels = true;
   GoogleStyle.KeepEmptyLinesAtTheStartOfBlocks = false;
   GoogleStyle.ObjCSpaceAfterProperty = false;
   GoogleStyle.ObjCSpaceBeforeProtocolList = false;
   GoogleStyle.PointerAlignment = FormatStyle::PAS_Left;
+  GoogleStyle.ReferenceAlignment = FormatStyle::RAS_Pointer;
   GoogleStyle.SpacesBeforeTrailingComments = 2;
   GoogleStyle.Standard = FormatStyle::LS_Auto;
 
   GoogleStyle.PenaltyReturnTypeOnItsOwnLine = 200;
   GoogleStyle.PenaltyBreakBeforeFirstCallParameter = 1;
 
   if (Language == FormatStyle::LK_Java) {
     GoogleStyle.AlignAfterOpenBracket = FormatStyle::BAS_DontAlign;
FormatStyle getMozillaStyle() {
   MozillaStyle.ContinuationIndentWidth = 2;
   MozillaStyle.Cpp11BracedListStyle = false;
   MozillaStyle.FixNamespaceComments = false;
   MozillaStyle.IndentCaseLabels = true;
   MozillaStyle.ObjCSpaceAfterProperty = true;
   MozillaStyle.ObjCSpaceBeforeProtocolList = false;
   MozillaStyle.PenaltyReturnTypeOnItsOwnLine = 200;
   MozillaStyle.PointerAlignment = FormatStyle::PAS_Left;
+  MozillaStyle.ReferenceAlignment = FormatStyle::RAS_Pointer;
   MozillaStyle.SpaceAfterTemplateKeyword = false;
   return MozillaStyle;
 }
 
 FormatStyle getWebKitStyle() {
   FormatStyle Style = getLLVMStyle();
   Style.AccessModifierOffset = -4;
   Style.AlignAfterOpenBracket = FormatStyle::BAS_DontAlign;
   Style.AlignOperands = false;
   Style.AlignTrailingComments = false;
   Style.BreakBeforeBinaryOperators = FormatStyle::BOS_All;
   Style.BreakBeforeBraces = FormatStyle::BS_WebKit;
   Style.BreakConstructorInitializersBeforeComma = true;
   Style.Cpp11BracedListStyle = false;
   Style.ColumnLimit = 0;
   Style.FixNamespaceComments = false;
   Style.IndentWidth = 4;
   Style.NamespaceIndentation = FormatStyle::NI_Inner;
   Style.ObjCBlockIndentWidth = 4;
   Style.ObjCSpaceAfterProperty = true;
   Style.PointerAlignment = FormatStyle::PAS_Left;
+  Style.ReferenceAlignment = FormatStyle::RAS_Pointer;
   return Style;
 }
 
 FormatStyle getGNUStyle() {
   FormatStyle Style = getLLVMStyle();
   Style.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_All;
   Style.AlwaysBreakAfterReturnType = FormatStyle::RTBS_AllDefinitions;
   Style.BreakBeforeBinaryOperators = FormatStyle::BOS_All;

lib/Format/TokenAnnotator.h

private:
   bool canBreakBefore(const AnnotatedLine &Line, const FormatToken &Right);
 
   bool mustBreakForReturnType(const AnnotatedLine &Line) const;
 
   void printDebugInfo(const AnnotatedLine &Line);
 
   void calculateUnbreakableTailLengths(AnnotatedLine &Line);
 
+  FormatStyle::PointerAlignmentStyle getTokenPointerAlignment(const FormatToken &Token);
+
   const FormatStyle &Style;
 
   const AdditionalKeywords &Keywords;
 };
 
 } // end namespace format
 } // end namespace clang
 
 #endif

lib/Format/TokenAnnotator.cpp

return Left.Tok.isLiteral() || (Left.is(tok::identifier) && Left.Previous &&
                                     Left.Previous->is(tok::kw_case));
   if (Left.is(tok::l_square) && Right.is(tok::amp))
     return false;
   if (Right.is(TT_PointerOrReference))
     return (Left.is(tok::r_paren) && Line.MightBeFunctionDecl) ||
            (Left.Tok.isLiteral() || (Left.is(tok::kw_const) && Left.Previous &&
                                      Left.Previous->is(tok::r_paren)) ||
             (!Left.isOneOf(TT_PointerOrReference, tok::l_paren) &&
-             (Style.PointerAlignment != FormatStyle::PAS_Left ||
+             (getTokenPointerAlignment(Right) != FormatStyle::PAS_Left ||
               (Line.IsMultiVariableDeclStmt &&
                (Left.NestingLevel == 0 ||
                 (Left.NestingLevel == 1 && Line.First->is(tok::kw_for)))))));
   if (Right.is(TT_FunctionTypeLParen) && Left.isNot(tok::l_paren) &&
       (!Left.is(TT_PointerOrReference) ||
-       (Style.PointerAlignment != FormatStyle::PAS_Right &&
+       (getTokenPointerAlignment(Left) != FormatStyle::PAS_Right &&
         !Line.IsMultiVariableDeclStmt)))
     return true;
   if (Left.is(TT_PointerOrReference))
     return Right.Tok.isLiteral() || Right.is(TT_BlockComment) ||
            (Right.isOneOf(Keywords.kw_override, Keywords.kw_final) &&
             !Right.is(TT_StartOfName)) ||
            (Right.is(tok::l_brace) && Right.BlockKind == BK_Block) ||
            (!Right.isOneOf(TT_PointerOrReference, TT_ArraySubscriptLSquare,
                            tok::l_paren) &&
-            (Style.PointerAlignment != FormatStyle::PAS_Right &&
+            (getTokenPointerAlignment(Left) != FormatStyle::PAS_Right &&
              !Line.IsMultiVariableDeclStmt) &&
             Left.Previous &&
             !Left.Previous->isOneOf(tok::l_paren, tok::coloncolon));
   if (Right.is(tok::star) && Left.is(tok::l_paren))
     return false;
   if (Left.is(tok::l_square))
     return (Left.is(TT_ArrayInitializerLSquare) &&
             Style.SpacesInContainerLiterals && Right.isNot(tok::r_square)) ||
bool TokenAnnotator::canBreakBefore(const AnnotatedLine &Line,
   if (Left.is(tok::at))
     return false;
   if (Left.Tok.getObjCKeywordID() == tok::objc_interface)
     return false;
   if (Left.isOneOf(TT_JavaAnnotation, TT_LeadingJavaAnnotation))
     return !Right.is(tok::l_paren);
   if (Right.is(TT_PointerOrReference))
     return Line.IsMultiVariableDeclStmt ||
-           (Style.PointerAlignment == FormatStyle::PAS_Right &&
+           (getTokenPointerAlignment(Right) == FormatStyle::PAS_Right &&
             (!Right.Next || Right.Next->isNot(TT_FunctionDeclarationName)));
   if (Right.isOneOf(TT_StartOfName, TT_FunctionDeclarationName) ||
       Right.is(tok::kw_operator))
     return true;
   if (Left.is(TT_PointerOrReference))
     return false;
   if (Right.isTrailingComment())
     // We rely on MustBreakBefore being set correctly here as we should not
while (Tok) {
     llvm::errs() << " Text='" << Tok->TokenText << "'\n";
     if (!Tok->Next)
       assert(Tok == Line.Last);
     Tok = Tok->Next;
   }
   llvm::errs() << "----\n";
 }
 
+FormatStyle::PointerAlignmentStyle TokenAnnotator::getTokenPointerAlignment(const FormatToken &Token) {
+  if (Token.isOneOf(tok::amp, tok::ampamp)) {
+    switch (Style.ReferenceAlignment) {
+    case FormatStyle::RAS_Pointer:
+      return Style.PointerAlignment;
+    case FormatStyle::RAS_Left:
+      return FormatStyle::PAS_Left;
+    case FormatStyle::RAS_Right:
+      return FormatStyle::PAS_Right;
+    case FormatStyle::RAS_Middle:
+      return FormatStyle::PAS_Middle;
+    }
+  }
+  return Style.PointerAlignment;
+}
+
 } // namespace format
 } // namespace clang