[clang-format] Reference qualifiers in member templates causing extra indentation.

[AndWass]

Summary

The following code

struct f {
  template <class T>
  void bar() && noexcept {}
};

will be formatted to the following with LLVM style, and
AlwaysBreakTemplateDeclarations: Yes

struct f {
  template <class T>
      void bar() && noexcept {}
};

The indentation of the void bar() line is wrong.

[klimek]

LG, nice patch, thanks!

[AndWass]

@klimek I don't have commit access yet, could you please commit it for me? Thanks

[ilya-biryukov]

The patch breaks some tests, see
http://lab.llvm.org:8011/builders/clang-cmake-x86_64-sde-avx512-linux/builds/27891/steps/ninja%20check%201/logs/FAIL%3A%20Clang-Unit%3A%3AFormatTest.FunctionAnnotations

Happy to re-land when this is fixed.

[AndWass]

Updated to fix test failures.

[MyDeveloperDay]

As the number of clauses is vast, I think sometimes its worth just putting a one-line comment in to explain what construct you are matching, just to help future maintainers

clang/lib/Format/TokenAnnotator.cpp

1375

could this be done using the MatchingParen?

2704

are you not testing the volatile case? could you add a comment here as to what you are actually matching like the lines above

[AndWass]

Removed the manual paranthesis-tracking, using NestingLevel instead.
Moved tests to the already existing UnderstandsFunctionRefQualification test case.
Clarified what is being matched against in TokenAnnotator::spaceRequiredBetween

[AndWass]

clang/lib/Format/TokenAnnotator.cpp

1375

Removed the old code to use Current.NestingLevel instead.

2704

const and volatile are tested in existing tests. I moved my tests to the same test case.

[MyDeveloperDay]

LG

[AndWass]

Thanks! @MyDeveloperDay could you commit as well? Don't have access for that yet

clang/lib/Format/TokenAnnotator.cpp

 /// The \c TokenAnnotator tries to match parenthesis and square brakets and
 /// store a parenthesis levels. It also tries to resolve matching "<" and ">"
 /// into template parameter lists.
 class AnnotatingParser {
 public:
   AnnotatingParser(const FormatStyle &Style, AnnotatedLine &Line,
                    const AdditionalKeywords &Keywords)
       : Style(Style), Line(Line), CurrentToken(Line.First), AutoFound(false),
-        Keywords(Keywords) {
+        TrailingReturnFound(false), ParenthesisDepth(0), Keywords(Keywords) {
     Contexts.push_back(Context(tok::unknown, 1, /*IsExpression=*/false));
     resetTokenMetadata(CurrentToken);
   }
 
 private:
   bool parseAngle() {
     if (!CurrentToken || !CurrentToken->Previous)
       return false;
if (Style.Language == FormatStyle::LK_JavaScript) {
         if (Current.Next &&
             Current.Next->isOneOf(TT_BinaryOperator, Keywords.kw_as)) {
           Current.Type = TT_JsNonNullAssertion;
           return;
         }
       }
     }
 
+    if (Current.is(tok::l_paren)) {
+      ParenthesisDepth++;
+    } else if (Current.is(tok::r_paren)) {
+      ParenthesisDepth--;
+    }
+
     // Line.MightBeFunctionDecl can only be true after the parentheses of a
     // function declaration have been found. In this case, 'Current' is a
     // trailing token of this declaration and thus cannot be a name.
     if (Current.is(Keywords.kw_instanceof)) {
       Current.Type = TT_BinaryOperator;
     } else if (isStartOfName(Current) &&
                (!Line.MightBeFunctionDecl || Current.NestingLevel != 0)) {
       Contexts.back().FirstStartOfName = &Current;
       Current.Type = TT_StartOfName;
     } else if (Current.is(tok::semi)) {
       // Reset FirstStartOfName after finding a semicolon so that a for loop
       // with multiple increment statements is not confused with a for loop
       // having multiple variable declarations.
       Contexts.back().FirstStartOfName = nullptr;
     } else if (Current.isOneOf(tok::kw_auto, tok::kw___auto_type)) {
       AutoFound = true;
     } else if (Current.is(tok::arrow) &&
                Style.Language == FormatStyle::LK_Java) {
       Current.Type = TT_LambdaArrow;
     } else if (Current.is(tok::arrow) && AutoFound && Line.MustBeDeclaration &&
                Current.NestingLevel == 0) {
       Current.Type = TT_TrailingReturnArrow;
-    } else if (Current.isOneOf(tok::star, tok::amp, tok::ampamp)) {
+      TrailingReturnFound = true;
+    } else if (Current.is(tok::star) ||
+               (Current.isOneOf(tok::amp, tok::ampamp) &&
+                (ParenthesisDepth != 0 || !Line.MightBeFunctionDecl ||
+                 TrailingReturnFound))) {
       Current.Type = determineStarAmpUsage(Current,
                                            Contexts.back().CanBeExpression &&
                                                Contexts.back().IsExpression,
                                            Contexts.back().InTemplateArgument);
     } else if (Current.isOneOf(tok::minus, tok::plus, tok::caret)) {
       Current.Type = determinePlusMinusCaretUsage(Current);
       if (Current.is(TT_UnaryOperator) && Current.is(tok::caret))
         Contexts.back().CaretFound = true;
     } else if (Current.isOneOf(tok::minusminus, tok::plusplus)) {
       Current.Type = determineIncrementUsage(Current);
     } else if (Current.isOneOf(tok::exclaim, tok::tilde)) {
       Current.Type = TT_UnaryOperator;
     } else if (Current.is(tok::question)) {
       if (Style.Language == FormatStyle::LK_JavaScript &&
           Line.MustBeDeclaration && !Contexts.back().IsExpression) {
         // In JavaScript, `interface X { foo?(): bar; }` is an optional method
         // on the interface, not a ternary expression.
         Current.Type = TT_JsTypeOptionalQuestion;
       } else {
         Current.Type = TT_ConditionalExpr;
       }
     } else if (Current.isBinaryOperator() &&
+               !(Line.MightBeFunctionDecl && ParenthesisDepth == 0 &&
+                 Current.isOneOf(tok::amp, tok::ampamp)) &&
                (!Current.Previous || Current.Previous->isNot(tok::l_square)) &&
                (!Current.is(tok::greater) &&
                 Style.Language != FormatStyle::LK_TextProto)) {
       Current.Type = TT_BinaryOperator;
     } else if (Current.is(tok::comment)) {
       if (Current.TokenText.startswith("/*")) {
         if (Current.TokenText.endswith("*/"))
           Current.Type = TT_BlockComment;
if (Current.is(Keywords.kw_instanceof)) {
                Current.Previous->MatchingParen &&
                Current.Previous->MatchingParen->Previous &&
                Current.Previous->MatchingParen->Previous->is(
                    TT_ObjCMethodSpecifier)) {
       // This is the first part of an Objective-C selector name. (If there's no
       // colon after this, this is the only place which annotates the identifier
       // as a selector.)
       Current.Type = TT_SelectorName;
-    } else if (Current.isOneOf(tok::identifier, tok::kw_const) &&
+    } else if (Current.isOneOf(tok::identifier, tok::kw_const, tok::amp,
+                               tok::ampamp) &&
                Current.Previous &&
                !Current.Previous->isOneOf(tok::equal, tok::at) &&
-               Line.MightBeFunctionDecl && Contexts.size() == 1) {
+               Line.MightBeFunctionDecl && !TrailingReturnFound &&
+               Contexts.size() == 1 && ParenthesisDepth == 0) {
       // Line.MightBeFunctionDecl can only be true after the parentheses of a
       // function declaration have been found.
       Current.Type = TT_TrailingAnnotation;
     } else if ((Style.Language == FormatStyle::LK_Java ||
                 Style.Language == FormatStyle::LK_JavaScript) &&
                Current.Previous) {
       if (Current.Previous->is(tok::at) &&
           Current.isNot(Keywords.kw_interface)) {
private:
   }
 
   SmallVector<Context, 8> Contexts;
 
   const FormatStyle &Style;
   AnnotatedLine &Line;
   FormatToken *CurrentToken;
   bool AutoFound;
+  bool TrailingReturnFound;
+  int ParenthesisDepth;
   const AdditionalKeywords &Keywords;
 
   // Set of "<" tokens that do not open a template parameter list. If parseAngle
   // determines that a specific token can't be a template opener, it will make
   // same decision irrespective of the decisions for tokens leading up to it.
   // Store this information to prevent this from causing exponential runtime.
   llvm::SmallPtrSet<FormatToken *, 16> NonTemplateLess;
 };
if (Left.is(TT_TemplateCloser) && Right.is(tok::l_square))
     return false;
   if (Left.is(tok::l_brace) && Left.endsSequence(TT_DictLiteral, tok::at))
     // Objective-C dictionary literal -> no space after opening brace.
     return false;
   if (Right.is(tok::r_brace) && Right.MatchingParen &&
       Right.MatchingParen->endsSequence(TT_DictLiteral, tok::at))
     // Objective-C dictionary literal -> no space before closing brace.
     return false;
+  if (Right.Type == TT_TrailingAnnotation &&
+      Right.isOneOf(tok::amp, tok::ampamp) &&
+      Left.isOneOf(tok::kw_const, tok::kw_volatile) &&
+      (!Right.Next || Right.Next->is(tok::semi)))
+    return Style.PointerAlignment != FormatStyle::PAS_Left;
   return true;
 }
 
 bool TokenAnnotator::spaceRequiredBefore(const AnnotatedLine &Line,
                                          const FormatToken &Right) {
   const FormatToken &Left = *Right.Previous;
   if (Right.Tok.getIdentifierInfo() && Left.Tok.getIdentifierInfo())
     return true; // Never ever merge two identifiers.
   if (Style.isCpp()) {

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, AmbersandInLamda) {
   // Test case reported in https://bugs.llvm.org/show_bug.cgi?id=41899
   FormatStyle AlignStyle = getLLVMStyle();
   AlignStyle.PointerAlignment = FormatStyle::PAS_Left;
   verifyFormat("auto lambda = [&a = a]() { a = 2; };", AlignStyle);
   AlignStyle.PointerAlignment = FormatStyle::PAS_Right;
   verifyFormat("auto lambda = [&a = a]() { a = 2; };", AlignStyle);
 }
 
+TEST_F(FormatTest, RefQualifiedTemplateMember) {
+  FormatStyle AlignStyle = getLLVMStyle();
+  AlignStyle.AlwaysBreakTemplateDeclarations = FormatStyle::BTDS_Yes;
+
+  verifyFormat("struct f {\n"
+               "  template <class T>\n"
+               "  int &foo() & noexcept {}\n"
+               "};",
+               AlignStyle);
+
+  verifyFormat("struct f {\n"
+               "  template <class T>\n"
+               "  int &foo() && noexcept {}\n"
+               "};",
+               AlignStyle);
+
+  verifyFormat("struct f {\n"
+               "  template <class T>\n"
+               "  int &foo() const & noexcept {}\n"
+               "};",
+               AlignStyle);
+
+  verifyFormat("struct f {\n"
+               "  template <class T>\n"
+               "  int &foo() const & noexcept {}\n"
+               "};",
+               AlignStyle);
+
+  verifyFormat("struct f {\n"
+               "  template <class T>\n"
+               "  auto foo() && noexcept -> int & {}\n"
+               "};",
+               AlignStyle);
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang