Bisected to a84e0b4. @gedare can you have a look?

The root of the problem here is that clang-format uses the FunctionTypeLParen to infer a function pointer from something that looks like a function declaration. So the assignment of a function pointer to a variable is triggering logic for both AlignConsecutiveAssignments and AlignConsecutiveDeclarations, and their interaction is resulting in this regression.

Disabling alignment of assignments, there is also some difference between these versions of clang-format despite that my commit only modifies the AlignConsecutiveDeclarations part:

newer version:

clang-format -style="{AlignConsecutiveAssignments: false, AlignConsecutiveDeclarations: true}" test.cpp
struct A {};

using Fn = int   (A::*)();
using RFn = int  (A::*)() &;
using RRFn = int (A::*)() &&;

older version:

clang-format -style="{AlignConsecutiveAssignments: false, AlignConsecutiveDeclarations: true}" test.cpp
struct A {};

using Fn = int (A::*)();
using RFn = int (A::*)() &;
using RRFn = int (A::*)() &&;

I'm not sure, but I think the AlignConsecutiveDeclarations should be ignoring text after the assignment? Is there a case in which an actual declaration can be a right-hand-side of an assignment?

I can try to work on a fix to better isolate declarations and assignments. The documentation is a bit confusing about what the differences are between them. https://clang.llvm.org/docs/ClangFormatStyleOptions.html#alignconsecutivedeclarations

In looking at the regression, it appears to be that the AlignConsecutiveDeclarations pass happens first, and then AlignConsecutiveAssignments, and this results in the ugly formatting.

I just did a quick test of

index dc81060671c1..41b1f7f59acf 100644
--- a/clang/lib/Format/WhitespaceManager.cpp
+++ b/clang/lib/Format/WhitespaceManager.cpp
@@ -108,9 +108,9 @@ const tooling::Replacements &WhitespaceManager::generateReplacements() {
   calculateLineBreakInformation();
   alignConsecutiveMacros();
   alignConsecutiveShortCaseStatements();
-  alignConsecutiveDeclarations();
   alignConsecutiveBitFields();
   alignConsecutiveAssignments();
+  alignConsecutiveDeclarations();
   alignChainedConditionals();
   alignTrailingComments();
   alignEscapedNewlines();

and this gives:

clang-format -style="{AlignConsecutiveAssignments: true, AlignConsecutiveDeclarations: true}" test.cpp
struct A {};

using Fn   = int (A::*)();
using RFn  = int (A::*)() &;
using RRFn = int (A::*)() &&;

This appears to fix the regression., but it breaks other tests. I'll look into finding a better fix but not sure when I can devote solid time to it.

I will revert a84e0b4 until we can come up with a good solution. We also must decide whether aligning on the l_paren of the function pointer is correct as the documentation says to align on the name instead.

I will revert a84e0b4 until we can come up with a good solution.

I have an OK solution. It may need a little reworking. I'll submit a rev shortly.

We also must decide whether aligning on the l_paren of the function pointer is correct as the documentation says to align on the name instead.

Where does it say to align on the name?

Actually, the documentation for AlignConsecutiveDeclarations appears to be mixed with AlignConsecutiveMacros.

I will revert a84e0b4 until we can come up with a good solution.

I have an OK solution. It may need a little reworking. I'll submit a rev shortly.

Thanks! Nevertheless, we should, IMO, still revert the offending patch for 17.x as your new solution, if merged, would be too late to be backported. What do you think, @mydeveloperday, @HazardyKnusperkeks, and @rymiel?

We also must decide whether aligning on the l_paren of the function pointer is correct as the documentation says to align on the name instead.

Where does it say to align on the name?

From the documentation I linked above: Consecutive will align the declaration names of consecutive lines.

It seems more consistent to align the function pointer names just like any pointer/reference names. For example:

int   i;
int  &r;
int  *p;
int (*f)();

17.0.3 is "just" released, so for that everything is too late. No need for a revert.

My question to the tests of #69340 is: Do we want to align the "declarations" after the equal? That aren't actually declarations, or am I wrong?

17.0.3 is "just" released, so for that everything is too late. No need for a revert.

@tru would it be possible for the revert to go into 17.0.4 if we decide to revert a84e0b4?

My question to the tests of #69340 is: Do we want to align the "declarations" after the equal? That aren't actually declarations, or am I wrong?

You are right. In fact, I was going to make a quick fix by excluding using statements before I realized it would be too late to backport any new fixes other than a reversion.

My question to the tests of #69340 is: Do we want to align the "declarations" after the equal? That aren't actually declarations, or am I wrong?

You are right. In fact, I was going to make a quick fix by excluding using statements before I realized it would be too late to backport any new fixes other than a reversion.

That's a very good point. It can be reduced down similar to this example to whether it is preferred to have:

using Fn   = int    (A::*)();
using RFn  = int   *(A::*)() &;
using RRFn = double (A::*)() &&;

or to have

using Fn   = int (A::*)();
using RFn  = int *(A::*)() &;
using RRFn = double (A::*)() &&;

The OP issue does not have this problem. I think it is outside of the current documentation, but the second option is consistent with the notion that a function pointer is not a declaration.

Are there any declarations that come after the equal operator? If not, then the fix is pretty simple:

-- a/clang/lib/Format/WhitespaceManager.cpp
+++ b/clang/lib/Format/WhitespaceManager.cpp
@@ -980,6 +980,9 @@ void WhitespaceManager::alignConsecutiveDeclarations() {
   AlignTokens(
       Style,
       [](Change const &C) {
+        for (FormatToken *Prev = C.Tok->Previous; Prev; Prev = Prev->Previous)
+          if (Prev->is(tok::equal))
+            return false;
         if (C.Tok->isOneOf(TT_FunctionDeclarationName, TT_FunctionTypeLParen))
           return true;
         if (C.Tok->isNot(TT_StartOfName))

Are there any declarations that come after the equal operator? If not, then the fix is pretty simple:

Not that I know of. Only inside a lambda, but I don't think that we would hit that.

-- a/clang/lib/Format/WhitespaceManager.cpp
+++ b/clang/lib/Format/WhitespaceManager.cpp
@@ -980,6 +980,9 @@ void WhitespaceManager::alignConsecutiveDeclarations() {
   AlignTokens(
       Style,
       [](Change const &C) {
+        for (FormatToken *Prev = C.Tok->Previous; Prev; Prev = Prev->Previous)
+          if (Prev->is(tok::equal))
+            return false;
         if (C.Tok->isOneOf(TT_FunctionDeclarationName, TT_FunctionTypeLParen))
           return true;
         if (C.Tok->isNot(TT_StartOfName))

Would be nice if there is another solution with a O(1) check, instead of that loop. But I'll accept it.

Are there any declarations that come after the equal operator? If not, then the fix is pretty simple:

Not that I know of. Only inside a lambda, but I don't think that we would hit that.

-- a/clang/lib/Format/WhitespaceManager.cpp
+++ b/clang/lib/Format/WhitespaceManager.cpp
@@ -980,6 +980,9 @@ void WhitespaceManager::alignConsecutiveDeclarations() {
   AlignTokens(
       Style,
       [](Change const &C) {
+        for (FormatToken *Prev = C.Tok->Previous; Prev; Prev = Prev->Previous)
+          if (Prev->is(tok::equal))
+            return false;
         if (C.Tok->isOneOf(TT_FunctionDeclarationName, TT_FunctionTypeLParen))
           return true;
         if (C.Tok->isNot(TT_StartOfName))

Would be nice if there is another solution with a O(1) check, instead of that loop. But I'll accept it.

I pushed this approach to the PR and I think it's ready to review and hopefully merge. I agree but I didn't think of any. The required information is not available at the line level to filter out lines once they've been partially aligned.

IMO, because a84e0b4 was kind of a new feature (aligning function pointers with other declaration names) and caused a regression in 17.x, we should revert it in 17.0.4 (if it's ok by @tru). We can then add it in 18.x after reaching a consensus on whether to align the left parenthesis or the name. We definitely need input from @mydeveloperday on this.

I'm reverting a84e0b4 for 17.x while we are deciding on how to move forward with aligning function pointers.

/cherry-pick 7bc1031

/branch llvm/llvm-project-release-prs/issue68079

/pull-request llvm/llvm-project-release-prs#747

From the documentation I linked above: Consecutive will align the declaration names of consecutive lines.

It seems more consistent to align the function pointer names just like any pointer/reference names. For example:

int   i;
int  &r;
int  *p;
int (*f)();

Aligning on the function pointer name is non-trivial. There can be complex conditions such as functions that return function pointers, in which case you end up with several nesting levels of parentheses and stars. I'm not confident that a simple solution exists that can manipulate the alignment shift in the whitespace.

Now I think aligning the left parenthesis is the correct thing to do:

int   i;
int  &r;
int &&rr;
int  *p;
int **pp;
int   (*f)();
      ctor();
      ~dtor();

We just need an option to turn it on/off.