[Format] Don't derive pointers right based on space before method ref-qualifiers

[sammccall]

Summary

The second space in void foo() & is always produced by clang-format,
and isn't evidence of any particular style.

Before this patch, it was considered evidence of PAS_Right, because
there is a space before a pointerlike ampersand.

This caused the following code to have "unstable" pointer alignment:

void a() &;
void b() &;
int *x;

PAS_Left, Derive=false would produce 'int* x' with other lines unchanged.
But subsequent formatting with Derive=true would produce 'int *x' again.

[sammccall]

clean up formatting

[benhamilton]

clang/lib/Format/Format.cpp

1949

Do we also need to worry about T && foo(); style declarations?

1951

If this is parsing a general function declaration, I think it's not safe to assume that the token immediately before the ref-spec is the close-paren.

It looks like Prev can be a few other things, including at least:

• tok::kw_const
• tok::kw_volatile
• tok::kw_throw
• tok::kw_noexcept

and probably C++ attribute(s) as well:

noptr-declarator ( parameter-list ) cv(optional) ref(optional) except(optional) attr(optional)

• https://en.cppreference.com/w/cpp/language/function
• https://en.cppreference.com/w/cpp/language/declarations

I *think* they can also be in any order, so the ref-spec could come at the very end after cv-qualifier, an expection specification, and C++ attributes.

[sammccall]

clang/lib/Format/Format.cpp

1949

A space before && in that case *should* trigger right-alignment.

Are you concerned the exclusion here might fire?

1951

const/volatile: a space between const and & does indicate right alignment. (And clang-format emits a space under PAS_Right)

throw etc: these are not possible, there's no "in any order". Here's the grammar: http://eel.is/c++draft/dcl.decl.general#nt:parameters-and-qualifiers

[benhamilton]

Got it, thanks.

clang/lib/Format/Format.cpp

1949

No, just wanted to make sure it should trigger right-alignment (which is what this implements, of course.)

1951

Ah, good to know the grammar is more strict about ordering.

clang/unittests/Format/FormatTest.cpp

9689–9692

Worth a test with &&?

[sammccall]

Thanks!

clang/unittests/Format/FormatTest.cpp

9689–9692

Good idea, merged it with the overloaded-operator test.

clang/lib/Format/Format.cpp

 //===----------------------------------------------------------------------===//
 
 #include "clang/Format/Format.h"
 #include "AffectedRangeManager.h"
 #include "BreakableToken.h"
 #include "ContinuationIndenter.h"
 #include "DefinitionBlockSeparator.h"
 #include "FormatInternal.h"
+#include "FormatToken.h"
 #include "FormatTokenLexer.h"
 #include "NamespaceEndCommentsFixer.h"
 #include "QualifierAlignmentFixer.h"
 #include "SortJavaScriptImports.h"
 #include "TokenAnalyzer.h"
 #include "TokenAnnotator.h"
 #include "UnwrappedLineFormatter.h"
 #include "UnwrappedLineParser.h"
private:
 
   int countVariableAlignments(const SmallVectorImpl<AnnotatedLine *> &Lines) {
     int AlignmentDiff = 0;
     for (const AnnotatedLine *Line : Lines) {
       AlignmentDiff += countVariableAlignments(Line->Children);
       for (FormatToken *Tok = Line->First; Tok && Tok->Next; Tok = Tok->Next) {
         if (!Tok->is(TT_PointerOrReference))
           continue;
+        // Don't treat space in `void foo() &&` as evidence.
+        if (const auto *Prev = Tok->getPreviousNonComment()) {
+          if (Prev->is(tok::r_paren) && Prev->MatchingParen)
+            if (const auto *Func = Prev->MatchingParen->getPreviousNonComment())
+              if (Func->isOneOf(TT_FunctionDeclarationName, TT_StartOfName,
+                                TT_OverloadedOperator))
+                continue;
+        }
         bool SpaceBefore = Tok->hasWhitespaceBefore();
         bool SpaceAfter = Tok->Next->hasWhitespaceBefore();
         if (SpaceBefore && !SpaceAfter)
           ++AlignmentDiff;
         if (!SpaceBefore && SpaceAfter)
           --AlignmentDiff;
       }
     }

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, UnderstandsFunctionRefQualification) {
   verifyFormat("struct f {\n"
                "  template <class T>\n"
                "  DEPRECATED(reason)\n"
                "  Type& foo(arguments) {}\n"
                "};",
                AlignLeftBreakTemplate);
 
   verifyFormat("void (*foopt)(int) = &func;");
+
+  FormatStyle DerivePointerAlignment = getLLVMStyle();
+  DerivePointerAlignment.DerivePointerAlignment = true;
+  // There's always a space between the function and its trailing qualifiers.
+  // This isn't evidence for PAS_Right (or for PAS_Left).
+  std::string Prefix = "void a() &;\n"
+                       "void b() &;\n";
+  verifyFormat(Prefix + "int* x;", DerivePointerAlignment);
+  verifyFormat(Prefix + "int *x;", DerivePointerAlignment);
+  // Same if the function is an overloaded operator instead.
+  Prefix = "void operator()() &;\n"
+           "void operator()() &;\n";
+  verifyFormat(Prefix + "int* x;", DerivePointerAlignment);
+  verifyFormat(Prefix + "int *x;", DerivePointerAlignment);
+  // However a space between cv-qualifiers and ref-qualifiers *is* evidence.
+  Prefix = "void a() const &;\n"
+           "void b() const &;\n";
+  EXPECT_EQ(Prefix + "int *x;",
+            format(Prefix + "int* x;", DerivePointerAlignment));
 }
 
 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"