Fix spacing for function with ref-qualification when using SpacesInCStyleCastParentheses != SpacesInParentheses

[jeanphilippeD]

Summary

If the approach is not right, I'll try to rework the fix with your feedback.

Problem:

FormatStyle Spaces = getLLVMStyle();
verifyFormat("Deleted &operator=(const Deleted &)& = default;");

Spaces.SpacesInParentheses= true;
verifyFormat("Deleted( const Deleted & )& = default;", Spaces); // Fail "Deleted(const Deleted &)& = default;"

Spaces.SpacesInCStyleCastParentheses = true;
Spaces.SpacesInParentheses= false;
verifyFormat("Deleted(const Deleted &)& = default;", Spaces); // Fail "Deleted( const Deleted & )& = default;"

Solution:

The cast seemed too eager: There does not seem to be a reason for a binary operator to follow a C-cast.

The type of the function reference qualification seems to make sense as TT_PointerOrReference:
-Extend in the case of to handle the '&&' case "Deleted(const Deleted &)&& = default;"
-Extend to handle the case without '=' for '*', '&' and '&&' case "Deleted(const Deleted &)&;"

Ensure no spaces between parenthesis and the reference qualification:
"Deleted &operator=(const Deleted &)&;" can be detected with TT_OverloadedOperatorLParen
"SomeType MemberFunction(const Deleted &)&;" can be detected with TT_FunctionDeclarationName

"Deleted(const Deleted &)&;" does not exist as ref-qualification does not make sense for constructors

Tests:

The set of unit tests is not quite minimal, it needs:

• To cover LLVM, Google, SpacesInCStyleCastParentheses, SpacesInParentheses.
• Both ref-qualifications : '&' and '&&'
• Operator, function, and constructor.

Additional test:
Verify clang-format output is unchanged for the 3 modified files FormatToken.h, TokenAnnotator.cpp, FormatTest.cpp

[djasper]

lib/Format/FormatToken.h

46 ↗

(On Diff #20467)

I am not convinced that we actually need a separate token type to fix this.

lib/Format/TokenAnnotator.cpp

463

I think, this is wrong. "<identifier>(" can be different things.

769–771

Which of the new tests does this affect?

805

I think a */& before a semicolon is always a pointer/reference, no?

There is a test in determineStarAmpUsage:

if (NextToken->isOneOf(tok::kw_operator, tok::comma))

I think we can just add semi to that list instead of this change.

975

I think this must have been there for a reason. What broke when you just removed this?

1665–1673

In what cases do we want a space between ")" and a pointer/references? The case where it is a TT_CastRParen is already handled at this point. So my question is, what happens if you remove everything of what you have added except "Left.is(tok::r_paren)"?

[jeanphilippeD]

Update as per review comment

Do not handle constructors anymore since there is no ref-qualification for constructors:
Remove TT_FunctionLParen since it is no longer needed.

Move the handling of '&;' to determineStarAmpUsage as per comment.

Kept restriction to operator overload and function to attach pointer reference to right parenthesis, rewrite without TT_FunctionLParen.

[jeanphilippeD]

Thank you very much for the feedback.
I already published an updated patch.
I reply inline to the questions you raised during the review.

lib/Format/TokenAnnotator.cpp

769–771

The ones with the && at the end of the operator:

[ RUN      ] FormatTest.UnderstandsFunctionRefQualification
Actual: "Deleted &operator=(const Deleted &) && = delete;"
Expected: "Deleted &operator=(const Deleted &)&& = delete;"
Actual: "SomeType MemberFunction(const Deleted &) && = delete;"
Expected: "SomeType MemberFunction(const Deleted &)&& = delete;"
[  FAILED  ] FormatTest.UnderstandsFunctionRefQualification (140 ms)

975

Removing this from master just break these tests:

[ RUN      ] FormatTest.UnderstandsOverloadedOperators
Actual: "Deleted &operator=(const Deleted &) & = default;"
Expected: "Deleted &operator=(const Deleted &)& = default;"
Actual: "Deleted &operator=(const Deleted &) && = delete;"
Expected:"Deleted &operator=(const Deleted &)&& = delete;"
Actual: "Deleted& operator=(const Deleted&) && = delete;"
Expected:"Deleted& operator=(const Deleted&)&& = delete;"
[  FAILED  ] FormatTest.UnderstandsOverloadedOperators (234 ms)
[----------] Global test environment tear-down
[==========] 315 tests from 4 test cases ran. (47166 ms total)
[  PASSED  ] 314 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] FormatTest.UnderstandsOverloadedOperators

1665–1673

Ran with:

if (Right.is(TT_PointerOrReference))
  return !Left.is(tok::r_paren) &&
         (Left.Tok.isLiteral() ||
          (!Left.isOneOf(TT_PointerOrReference, tok::l_paren) &&
           Style.PointerAlignment != FormatStyle::PAS_Left));

[ RUN      ] FormatTest.UnderstandsUsesOfStarAndAmp
Actual: "typedef typeof(int(int, int))*MyFunc;"
Expected: "typedef typeof(int(int, int)) *MyFunc;"
Actual: "[](const decltype(*a)&value) {}"
Expected: "[](const decltype(*a) &value) {}"
[  FAILED  ] FormatTest.UnderstandsUsesOfStarAndAmp (1451 ms)

[djasper]

Looks good. Thanks for working on this. Do you have commit access?

[jeanphilippeD]

Thank you.
No I do not have commit access.
I'd prefer if you proceed with the commits as I'm still getting familiar
with the process.

2015-02-23 22:33 GMT+00:00 Daniel Jasper <[e-mail]>:

Looks good. Thanks for working on this. Do you have commit access?

http://reviews.llvm.org/D7813

EMAIL PREFERENCES

http://reviews.llvm.org/settings/panel/emailpreferences/

[djasper]

Submitted as r230473.

lib/Format/TokenAnnotator.cpp

case tok::kw_while:
       break;
     case tok::kw_for:
       Contexts.back().ColonIsForRangeExpr = true;
       next();
       if (!parseParens())
         return false;
       break;
     case tok::l_paren:
       if (!parseParens())
         return false;
       if (Line.MustBeDeclaration && Contexts.size() == 1 &&
           !Contexts.back().IsExpression && Line.First->isNot(TT_ObjCProperty) &&
           (!Tok->Previous ||
            !Tok->Previous->isOneOf(tok::kw_decltype, TT_LeadingJavaAnnotation)))
         Line.MightBeFunctionDecl = true;
       break;
     case tok::l_square:
if (Current.getPrecedence() == prec::Assignment &&
              Previous && !Previous->isOneOf(tok::comma, tok::semi);
              Previous = Previous->Previous) {
           if (Previous->isOneOf(tok::r_square, tok::r_paren)) {
             Previous = Previous->MatchingParen;
             if (!Previous)
               break;
           }
           if (Previous->isOneOf(TT_BinaryOperator, TT_UnaryOperator) &&
-              Previous->isOneOf(tok::star, tok::amp) && Previous->Previous &&
-              Previous->Previous->isNot(tok::equal))
+              Previous->isOneOf(tok::star, tok::amp, tok::ampamp) &&
+              Previous->Previous && Previous->Previous->isNot(tok::equal))
             Previous->Type = TT_PointerOrReference;
         }
       }
     } else if (Current.isOneOf(tok::kw_return, tok::kw_throw)) {
       Contexts.back().IsExpression = true;
     } else if (Current.is(TT_TrailingReturnArrow)) {
       Contexts.back().IsExpression = false;
     } else if (Current.is(tok::l_paren) && !Line.MustBeDeclaration &&
                !Line.InPPDirective &&
if (Current.getPrecedence() == prec::Assignment &&
                Current.Previous->is(TT_CtorInitializerColon)) {
       Contexts.back().IsExpression = true;
       Contexts.back().InCtorInitializer = true;
     } else if (Current.is(tok::kw_new)) {
       Contexts.back().CanBeExpression = false;
     } else if (Current.is(tok::semi) || Current.is(tok::exclaim)) {
       // This should be the condition or increment in a for-loop.
       Contexts.back().IsExpression = true;
     }
   }
 
   void determineTokenType(FormatToken &Current) {
     if (!Current.is(TT_Unknown))
       // The token type is already known.
       return;
 
     // Line.MightBeFunctionDecl can only be true after the parentheses of a
bool ParensAreType =
         !Tok.Previous ||
         Tok.Previous->isOneOf(TT_PointerOrReference, TT_TemplateCloser) ||
         Tok.Previous->isSimpleTypeSpecifier();
     bool ParensCouldEndDecl =
         Tok.Next && Tok.Next->isOneOf(tok::equal, tok::semi, tok::l_brace);
     bool IsSizeOfOrAlignOf =
         LeftOfParens && LeftOfParens->isOneOf(tok::kw_sizeof, tok::kw_alignof);
     if (ParensAreType && !ParensCouldEndDecl && !IsSizeOfOrAlignOf &&
-        ((Contexts.size() > 1 && Contexts[Contexts.size() - 2].IsExpression) ||
-         (Tok.Next && Tok.Next->isBinaryOperator())))
+        (Contexts.size() > 1 && Contexts[Contexts.size() - 2].IsExpression))
       IsCast = true;
     else if (Tok.Next && Tok.Next->isNot(tok::string_literal) &&
              (Tok.Next->Tok.isLiteral() ||
               Tok.Next->isOneOf(tok::kw_sizeof, tok::kw_alignof)))
       IsCast = true;
     // If there is an identifier after the (), it is likely a cast, unless
     // there is also an identifier before the ().
     else if (LeftOfParens && Tok.Next &&
if (PrevToken->isOneOf(tok::l_paren, tok::l_square, tok::l_brace,
                            tok::comma, tok::semi, tok::kw_return, tok::colon,
                            tok::equal, tok::kw_delete, tok::kw_sizeof) ||
         PrevToken->isOneOf(TT_BinaryOperator, TT_ConditionalExpr,
                            TT_UnaryOperator, TT_CastRParen))
       return TT_UnaryOperator;
 
     if (NextToken->is(tok::l_square) && NextToken->isNot(TT_LambdaLSquare))
       return TT_PointerOrReference;
-    if (NextToken->isOneOf(tok::kw_operator, tok::comma))
+    if (NextToken->isOneOf(tok::kw_operator, tok::comma, tok::semi))
       return TT_PointerOrReference;
 
     if (PrevToken->is(tok::r_paren) && PrevToken->MatchingParen &&
         PrevToken->MatchingParen->Previous &&
         PrevToken->MatchingParen->Previous->isOneOf(tok::kw_typeof,
                                                     tok::kw_decltype))
       return TT_PointerOrReference;
 
if (Left.is(tok::coloncolon))
     return false;
   if (Left.is(tok::less) || Right.isOneOf(tok::greater, tok::less))
     return false;
   if (Right.is(tok::ellipsis))
     return Left.Tok.isLiteral();
   if (Left.is(tok::l_square) && Right.is(tok::amp))
     return false;
   if (Right.is(TT_PointerOrReference))
-    return Left.Tok.isLiteral() ||
+    return !(Left.is(tok::r_paren) && Left.MatchingParen &&
+             (Left.MatchingParen->is(TT_OverloadedOperatorLParen) ||
+              (Left.MatchingParen->Previous &&
+               Left.MatchingParen->Previous->is(
+                   TT_FunctionDeclarationName)))) &&
+           (Left.Tok.isLiteral() ||
             (!Left.isOneOf(TT_PointerOrReference, tok::l_paren) &&
-            Style.PointerAlignment != FormatStyle::PAS_Left);
+             Style.PointerAlignment != FormatStyle::PAS_Left));
   if (Right.is(TT_FunctionTypeLParen) && Left.isNot(tok::l_paren) &&
       (!Left.is(TT_PointerOrReference) ||
        Style.PointerAlignment != FormatStyle::PAS_Right))
     return true;
   if (Left.is(TT_PointerOrReference))
     return Right.Tok.isLiteral() || Right.is(TT_BlockComment) ||
            (!Right.isOneOf(TT_PointerOrReference, tok::l_paren) &&
             Style.PointerAlignment != FormatStyle::PAS_Right && Left.Previous &&
             !Left.Previous->isOneOf(tok::l_paren, tok::coloncolon));

unittests/Format/FormatTest.cpp

TEST_F(FormatTest, UnderstandsOverloadedOperators) {
   verifyFormat("f.template operator()<int>();");
 
   verifyGoogleFormat("operator void*();");
   verifyGoogleFormat("operator SomeType<SomeType<int>>();");
   verifyGoogleFormat("operator ::A();");
 
   verifyFormat("using A::operator+;");
 
-  verifyFormat("Deleted &operator=(const Deleted &)& = default;");
-  verifyFormat("Deleted &operator=(const Deleted &)&& = delete;");
-  verifyGoogleFormat("Deleted& operator=(const Deleted&)& = default;");
-  verifyGoogleFormat("Deleted& operator=(const Deleted&)&& = delete;");
-
   verifyFormat("string // break\n"
                "operator()() & {}");
   verifyFormat("string // break\n"
                "operator()() && {}");
 }
 
+TEST_F(FormatTest, UnderstandsFunctionRefQualification) {
+  verifyFormat("Deleted &operator=(const Deleted &)& = default;");
+  verifyFormat("Deleted &operator=(const Deleted &)&& = delete;");
+  verifyFormat("SomeType MemberFunction(const Deleted &)& = delete;");
+  verifyFormat("SomeType MemberFunction(const Deleted &)&& = delete;");
+  verifyFormat("Deleted &operator=(const Deleted &)&;");
+  verifyFormat("Deleted &operator=(const Deleted &)&&;");
+  verifyFormat("SomeType MemberFunction(const Deleted &)&;");
+  verifyFormat("SomeType MemberFunction(const Deleted &)&&;");
+
+  verifyGoogleFormat("Deleted& operator=(const Deleted&)& = default;");
+  verifyGoogleFormat("SomeType MemberFunction(const Deleted&)& = delete;");
+  verifyGoogleFormat("Deleted& operator=(const Deleted&)&;");
+  verifyGoogleFormat("SomeType MemberFunction(const Deleted&)&;");
+
+  FormatStyle Spaces = getLLVMStyle();
+  Spaces.SpacesInCStyleCastParentheses = true;
+  verifyFormat("Deleted &operator=(const Deleted &)& = default;", Spaces);
+  verifyFormat("SomeType MemberFunction(const Deleted &)& = delete;", Spaces);
+  verifyFormat("Deleted &operator=(const Deleted &)&;", Spaces);
+  verifyFormat("SomeType MemberFunction(const Deleted &)&;", Spaces);
+
+  Spaces.SpacesInCStyleCastParentheses = false;
+  Spaces.SpacesInParentheses = true;
+  verifyFormat("Deleted &operator=( const Deleted & )& = default;", Spaces);
+  verifyFormat("SomeType MemberFunction( const Deleted & )& = delete;", Spaces);
+  verifyFormat("Deleted &operator=( const Deleted & )&;", Spaces);
+  verifyFormat("SomeType MemberFunction( const Deleted & )&;", Spaces);
+}
+
 TEST_F(FormatTest, UnderstandsNewAndDelete) {
   verifyFormat("void f() {\n"
                "  A *a = new A;\n"
                "  A *a = new (placement) A;\n"
                "  delete a;\n"
                "  delete (A *)a;\n"
                "}");
   verifyFormat("new (aaaaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaaaa))\n"