[clang-format] Added new option to allow setting spaces before and after the operator

[owenpan]

Summary

The new option is called SpaceBeforeAndAfterArrows

True:
(pointer instance) -> (member)

False:
(pointer instance)->(member)

[mitchell-stellar]

This feature is missing unit tests. Also, what is the reason for all the comment changes? I don't think the changed comment lines originally exceeded 80 characters.

[Luis]

In D71659#1789694, @mitchell-stellar wrote:

This feature is missing unit tests. Also, what is the reason for all the comment changes? I don't think the changed comment lines originally exceeded 80 characters.

Yup, I will add the unit-test when I have time, regarding the comments, it's really weird because I'm using clang-format to format the file on save, I'll double check and try to revert those comment changes!

[Bouska]

Is there a reason why this option adds space before and after the tok::arrow token but not the tok::arrowstar token? That seems inconsistent to me.

[MyDeveloperDay]

clang/lib/Format/TokenAnnotator.cpp

3359

these are all unrelated changes please remove them from patch

[rymiel]

NOTE: Clang-Format Team Automated Review Comment

Your review contains a change to clang/include/clang/Format/Format.h but does not contain an update to ClangFormatStyleOptions.rst

ClangFormatStyleOptions.rst is generated via clang/docs/tools/dump_format_style.py, please run this to regenerate the .rst

You can validate that the rst is valid by running.

./docs/tools/dump_format_style.py
mkdir -p html
/usr/bin/sphinx-build -n ./docs ./html

NOTE: Clang-Format Team Automated Review Comment

It looks like your clang-format review does not contain any unit tests, please try to ensure all code changes have a unit test (unless this is an NFC or refactoring, adding documentation etc..)

Add your unit tests in clang/unittests/Format and you can build with ninja FormatTests. We recommend using the verifyFormat(xxx) format of unit tests rather than EXPECT_EQ as this will ensure you change is tolerant to random whitespace changes (see FormatTest.cpp as an example)

For situations where your change is altering the TokenAnnotator.cpp which can happen if you are trying to improve the annotation phase to ensure we are correctly identifying the type of a token, please add a token annotator test in TokenAnnotatorTest.cpp

clang/include/clang/Format/Format.h

   /// If ``false``, spaces will be removed before assignment operators.
   /// \code
   ///    true:                                  false:
   ///    int a = 5;                     vs.     int a= 5;
   ///    a += 42;                               a+= 42;
   /// \endcode
   bool SpaceBeforeAssignmentOperators;
 
+  /// If ``false``, spaces will removed before and after arrow
+  /// \code
+  ///    true:                                  false:
+  ///    object -> member               vs      object->member
+  /// \endcode
+  bool SpaceBeforeAndAfterArrows;
+
   /// If ``true``, a space will be inserted before a C++11 braced list
   /// used to initialize an object (after the preceding identifier or type).
   /// \code
   ///    true:                                  false:
   ///    Foo foo { bar };               vs.     Foo foo{ bar };
   ///    Foo {};                                Foo{};
   ///    vector<int> { 1, 2, 3 };               vector<int>{ 1, 2, 3 };
   ///    new int[3] { 1, 2, 3 };                new int[3]{ 1, 2, 3 };
return AccessModifierOffset == R.AccessModifierOffset &&
            PenaltyReturnTypeOnItsOwnLine == R.PenaltyReturnTypeOnItsOwnLine &&
            PenaltyBreakTemplateDeclaration ==
                R.PenaltyBreakTemplateDeclaration &&
            PointerAlignment == R.PointerAlignment &&
            RawStringFormats == R.RawStringFormats &&
            SpaceAfterCStyleCast == R.SpaceAfterCStyleCast &&
            SpaceAfterLogicalNot == R.SpaceAfterLogicalNot &&
            SpaceAfterTemplateKeyword == R.SpaceAfterTemplateKeyword &&
+           SpaceBeforeAndAfterArrows == R.SpaceBeforeAndAfterArrows &&
            SpaceBeforeAssignmentOperators == R.SpaceBeforeAssignmentOperators &&
            SpaceBeforeCpp11BracedList == R.SpaceBeforeCpp11BracedList &&
            SpaceBeforeCtorInitializerColon ==
                R.SpaceBeforeCtorInitializerColon &&
            SpaceBeforeInheritanceColon == R.SpaceBeforeInheritanceColon &&
            SpaceBeforeParens == R.SpaceBeforeParens &&
            SpaceBeforeRangeBasedForLoopColon ==
                R.SpaceBeforeRangeBasedForLoopColon &&

clang/lib/Format/Format.cpp

static void mapping(IO &IO, FormatStyle &Style) {
     IO.mapOptional("RawStringFormats", Style.RawStringFormats);
     IO.mapOptional("ReflowComments", Style.ReflowComments);
     IO.mapOptional("SortIncludes", Style.SortIncludes);
     IO.mapOptional("SortUsingDeclarations", Style.SortUsingDeclarations);
     IO.mapOptional("SpaceAfterCStyleCast", Style.SpaceAfterCStyleCast);
     IO.mapOptional("SpaceAfterLogicalNot", Style.SpaceAfterLogicalNot);
     IO.mapOptional("SpaceAfterTemplateKeyword",
                    Style.SpaceAfterTemplateKeyword);
+    IO.mapOptional("SpaceBeforeAndAfterArrows",
+                   Style.SpaceBeforeAndAfterArrows);
     IO.mapOptional("SpaceBeforeAssignmentOperators",
                    Style.SpaceBeforeAssignmentOperators);
     IO.mapOptional("SpaceBeforeCpp11BracedList",
                    Style.SpaceBeforeCpp11BracedList);
     IO.mapOptional("SpaceBeforeCtorInitializerColon",
                    Style.SpaceBeforeCtorInitializerColon);
     IO.mapOptional("SpaceBeforeInheritanceColon",
                    Style.SpaceBeforeInheritanceColon);
FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
   LLVMStyle.SpacesInCStyleCastParentheses = false;
   LLVMStyle.SpaceAfterCStyleCast = false;
   LLVMStyle.SpaceAfterLogicalNot = false;
   LLVMStyle.SpaceAfterTemplateKeyword = true;
   LLVMStyle.SpaceBeforeCtorInitializerColon = true;
   LLVMStyle.SpaceBeforeInheritanceColon = true;
   LLVMStyle.SpaceBeforeParens = FormatStyle::SBPO_ControlStatements;
   LLVMStyle.SpaceBeforeRangeBasedForLoopColon = true;
+  LLVMStyle.SpaceBeforeAndAfterArrows = false;
   LLVMStyle.SpaceBeforeAssignmentOperators = true;
   LLVMStyle.SpaceBeforeCpp11BracedList = false;
   LLVMStyle.SpaceBeforeSquareBrackets = false;
   LLVMStyle.SpacesInAngles = false;
   LLVMStyle.SpacesInConditionalStatement = false;
 
   LLVMStyle.PenaltyBreakAssignment = prec::Assignment;
   LLVMStyle.PenaltyBreakComment = 300;

clang/lib/Format/TokenAnnotator.cpp

bool TokenAnnotator::spaceRequiredBefore(const AnnotatedLine &Line,
 
   if (Left.is(tok::greater) && Right.is(tok::greater)) {
     if (Style.Language == FormatStyle::LK_TextProto ||
         (Style.Language == FormatStyle::LK_Proto && Left.is(TT_DictLiteral)))
       return !Style.Cpp11BracedListStyle;
     return Right.is(TT_TemplateCloser) && Left.is(TT_TemplateCloser) &&
            (Style.Standard < FormatStyle::LS_Cpp11 || Style.SpacesInAngles);
   }
-  if (Right.isOneOf(tok::arrow, tok::arrowstar, tok::periodstar) ||
-      Left.isOneOf(tok::arrow, tok::period, tok::arrowstar, tok::periodstar) ||
+  if (Left.is(tok::arrow) || Right.is(tok::arrow)) {
+    return Style.SpaceBeforeAndAfterArrows;
+  }
+  if (Right.isOneOf(tok::arrowstar, tok::periodstar) ||
+      Left.isOneOf(tok::period, tok::arrowstar, tok::periodstar) ||
       (Right.is(tok::period) && Right.isNot(TT_DesignatedInitializerPeriod)))
     return false;
   if (!Style.SpaceBeforeAssignmentOperators && Left.isNot(TT_TemplateCloser) &&
       Right.getPrecedence() == prec::Assignment)
     return false;
   if (Style.Language == FormatStyle::LK_Java && Right.is(tok::coloncolon) &&
       (Left.is(tok::identifier) || Left.is(tok::kw_this)))
     return false;
   if (Right.is(tok::coloncolon) && Left.is(tok::identifier))
     // Generally don't remove existing spaces between an identifier and "::".
     // The identifier might actually be a macro name such as ALWAYS_INLINE. If
-    // this turns out to be too lenient, add analysis of the identifier itself.
+    // this turns out to be too lenient, add analysis of the identifier
+    // itself.
     return Right.WhitespaceRange.getBegin() != Right.WhitespaceRange.getEnd();
   if (Right.is(tok::coloncolon) &&
       !Left.isOneOf(tok::l_brace, tok::comment, tok::l_paren))
     return (Left.is(TT_TemplateOpener) &&
             Style.Standard < FormatStyle::LS_Cpp11) ||
            !(Left.isOneOf(tok::l_paren, tok::r_paren, tok::l_square,
                           tok::kw___super, TT_TemplateCloser,
                           TT_TemplateOpener)) ||
if (Right.is(tok::less) && Left.isNot(tok::l_paren) &&
     return true;
   if (Right.is(TT_TrailingUnaryOperator))
     return false;
   if (Left.is(TT_RegexLiteral))
     return false;
   return spaceRequiredBetween(Line, Left, Right);
 }
 
-// Returns 'true' if 'Tok' is a brace we'd want to break before in Allman style.
+// Returns 'true' if 'Tok' is a brace we'd want to break before in Allman
+// style.
 static bool isAllmanBrace(const FormatToken &Tok) {
   return Tok.is(tok::l_brace) && Tok.BlockKind == BK_Block &&
          !Tok.isOneOf(TT_ObjCBlockLBrace, TT_LambdaLBrace, TT_DictLiteral);
 }
 
 bool TokenAnnotator::mustBreakBefore(const AnnotatedLine &Line,
                                      const FormatToken &Right) {
   const FormatToken &Left = *Right.Previous;
   if (Right.NewlinesBefore > 1 && Style.MaxEmptyLinesToKeep > 0)
     return true;
 
   if (Style.Language == FormatStyle::LK_JavaScript) {
     // FIXME: This might apply to other languages and token kinds.
     if (Right.is(tok::string_literal) && Left.is(tok::plus) && Left.Previous &&
         Left.Previous->is(tok::string_literal))
       return true;
     if (Left.is(TT_DictLiteral) && Left.is(tok::l_brace) && Line.Level == 0 &&
         Left.Previous && Left.Previous->is(tok::equal) &&
         Line.First->isOneOf(tok::identifier, Keywords.kw_import, tok::kw_export,
                             tok::kw_const) &&
         // kw_var/kw_let are pseudo-tokens that are tok::identifier, so match
         // above.
         !Line.First->isOneOf(Keywords.kw_var, Keywords.kw_let))
-      // Object literals on the top level of a file are treated as "enum-style".
-      // Each key/value pair is put on a separate line, instead of bin-packing.
+      // Object literals on the top level of a file are treated as
+      // "enum-style". Each key/value pair is put on a separate line, instead
+      // of bin-packing.
       return true;
     if (Left.is(tok::l_brace) && Line.Level == 0 &&
         (Line.startsWith(tok::kw_enum) ||
          Line.startsWith(tok::kw_const, tok::kw_enum) ||
          Line.startsWith(tok::kw_export, tok::kw_enum) ||
          Line.startsWith(tok::kw_export, tok::kw_const, tok::kw_enum)))
       // JavaScript top-level enum key/value pairs are put on separate lines
       // instead of bin-packing.
if (Right.is(tok::plus) && Left.is(tok::string_literal) && Right.Next &&
              Style.Language == FormatStyle::LK_Proto ||
              Style.Language == FormatStyle::LK_TableGen ||
              Style.Language == FormatStyle::LK_TextProto) {
     if (Left.isStringLiteral() && Right.isStringLiteral())
       return true;
   }
 
   // If the last token before a '}', ']', or ')' is a comma or a trailing
-  // comment, the intention is to insert a line break after it in order to make
-  // shuffling around entries easier. Import statements, especially in
+  // comment, the intention is to insert a line break after it in order to
+  // make shuffling around entries easier. Import statements, especially in
   // JavaScript, can be an exception to this rule.
   if (Style.JavaScriptWrapImports || Line.Type != LT_ImportStatement) {
     const FormatToken *BeforeClosingBrace = nullptr;
     if ((Left.isOneOf(tok::l_brace, TT_ArrayInitializerLSquare) ||
          (Style.Language == FormatStyle::LK_JavaScript &&
           Left.is(tok::l_paren))) &&
         Left.BlockKind != BK_Block && Left.MatchingParen)
       BeforeClosingBrace = Left.MatchingParen->Previous;
if ((Style.Language == FormatStyle::LK_Java ||
       Left.is(TT_LeadingJavaAnnotation) &&
       Right.isNot(TT_LeadingJavaAnnotation) && Right.isNot(tok::l_paren) &&
       (Line.Last->is(tok::l_brace) || Style.BreakAfterJavaFieldAnnotations))
     return true;
 
   if (Right.is(TT_ProtoExtensionLSquare))
     return true;
 
-  // In text proto instances if a submessage contains at least 2 entries and at
-  // least one of them is a submessage, like A { ... B { ... } ... },
-  // put all of the entries of A on separate lines by forcing the selector of
-  // the submessage B to be put on a newline.
+  // In text proto instances if a submessage contains at least 2 entries and
+  // at least one of them is a submessage, like A { ... B { ... } ... }, put
+  // all of the entries of A on separate lines by forcing the selector of the
+  // submessage B to be put on a newline.
   //
   // Example: these can stay on one line:
   // a { scalar_1: 1 scalar_2: 2 }
   // a { b { key: value } }
   //
-  // and these entries need to be on a new line even if putting them all in one
-  // line is under the column limit:
-  // a {
+  // and these entries need to be on a new line even if putting them all in
+  // one line is under the column limit: a {
   //   scalar: 1
   //   b { key: value }
   // }
   //
   // We enforce this by breaking before a submessage field that has previous
   // siblings, *and* breaking before a field that follows a submessage field.
   //
   // Be careful to exclude the case  [proto.ext] { ... } since the `]` is
-  // the TT_SelectorName there, but we don't want to break inside the brackets.
+  // the TT_SelectorName there, but we don't want to break inside the
+  // brackets.
   //
   // Another edge case is @submessage { key: value }, which is a common
-  // substitution placeholder. In this case we want to keep `@` and `submessage`
-  // together.
+  // substitution placeholder. In this case we want to keep `@` and
+  // `submessage` together.
   //
   // We ensure elsewhere that extensions are always on their own line.
   if ((Style.Language == FormatStyle::LK_Proto ||
        Style.Language == FormatStyle::LK_TextProto) &&
       Right.is(TT_SelectorName) && !Right.is(tok::r_square) && Right.Next) {
     // Keep `@submessage` together in:
     // @submessage { key: value }
     if (Right.Previous && Right.Previous->is(tok::at))
if (LBrace &&
         // In case of selector { ... }, the l_brace is TT_DictLiteral.
         // In case of an empty selector {}, the l_brace is not TT_DictLiteral,
         // so we check for immediately following r_brace.
         ((LBrace->is(tok::l_brace) &&
           (LBrace->is(TT_DictLiteral) ||
            (LBrace->Next && LBrace->Next->is(tok::r_brace)))) ||
          LBrace->is(TT_ArrayInitializerLSquare) || LBrace->is(tok::less))) {
       // If Left.ParameterCount is 0, then this submessage entry is not the
-      // first in its parent submessage, and we want to break before this entry.
-      // If Left.ParameterCount is greater than 0, then its parent submessage
-      // might contain 1 or more entries and we want to break before this entry
-      // if it contains at least 2 entries. We deal with this case later by
-      // detecting and breaking before the next entry in the parent submessage.
+      // first in its parent submessage, and we want to break before this
+      // entry. If Left.ParameterCount is greater than 0, then its parent
+      // submessage might contain 1 or more entries and we want to break
+      // before this entry if it contains at least 2 entries. We deal with
+      // this case later by detecting and breaking before the next entry in
+      // the parent submessage.
       if (Left.ParameterCount == 0)
         return true;
       // However, if this submessage is the first entry in its parent
       // submessage, Left.ParameterCount might be 1 in some cases.
       // We deal with this case later by detecting an entry
       // following a closing paren of this submessage.
     }
 
     // If this is an entry immediately following a submessage, it will be
     // preceded by a closing paren of that submessage, like in:
     //     left---.  .---right
     //            v  v
     // sub: { ... } key: value
     // If there was a comment between `}` an `key` above, then `key` would be
     // put on a new line anyways.
     if (Left.isOneOf(tok::r_brace, tok::greater, tok::r_square))
       return true;
   }
 
-  // Deal with lambda arguments in C++ - we want consistent line breaks whether
-  // they happen to be at arg0, arg1 or argN. The selection is a bit nuanced
-  // as aggressive line breaks are placed when the lambda is not the last arg.
+  // Deal with lambda arguments in C++ - we want consistent line breaks
+  // whether they happen to be at arg0, arg1 or argN. The selection is a bit
+  // nuanced as aggressive line breaks are placed when the lambda is not the
+  // last arg.
   if ((Style.Language == FormatStyle::LK_Cpp ||
        Style.Language == FormatStyle::LK_ObjC) &&
       Left.is(tok::l_paren) && Left.BlockParameterCount > 0 &&
       !Right.isOneOf(tok::l_paren, TT_LambdaLSquare)) {
     // Multiple lambdas in the same function call force line breaks.
     if (Left.BlockParameterCount > 1)
       return true;
 
bool TokenAnnotator::canBreakBefore(const AnnotatedLine &Line,
   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
     // change the "binding" behavior of a comment.
-    // The first comment in a braced lists is always interpreted as belonging to
-    // the first list element. Otherwise, it should be placed outside of the
-    // list.
+    // The first comment in a braced lists is always interpreted as belonging
+    // to the first list element. Otherwise, it should be placed outside of
+    // the list.
     return Left.BlockKind == BK_BracedInit ||
            (Left.is(TT_CtorInitializerColon) &&
             Style.BreakConstructorInitializers == FormatStyle::BCIS_AfterColon);
   if (Left.is(tok::question) && Right.is(tok::colon))
     return false;
   if (Right.is(TT_ConditionalExpr) || Right.is(tok::question))
     return Style.BreakBeforeTernaryOperators;
   if (Left.is(TT_ConditionalExpr) || Left.is(tok::question))