[clang-format] Fix incorrect alignment of operator= overloads.

[glotchimo]

Summary

Fixes https://github.com/llvm/llvm-project/issues/31568.

Added a check for operator keyword tokens.

[glotchimo]

I don't know why, but clang-format reformatted most if not all of the long/block comments in WhitespaceManager.cpp. Will it be necessary for me to revert the changes to those comments and skip formatting when updating the diff so as to keep it minimal?

[glotchimo]

Same change as before but without the changes to long and block comments.

[curdeius]

Thanks for working on this!
I'll have a closer look tomorrow or on Tuesday.

clang/lib/Format/WhitespaceManager.cpp

735

Maybe getPreviousNonComment? Cf. test comment.

736

Please spell out the type instead of auto.

clang/unittests/Format/FormatTest.cpp

16172

Are other operators impacted too by this bug? Like ==, <=, !=.

16174

What happens if there's a block comment between operator and =?

[glotchimo]

Excellent, thank you! I'll get right on these edits.

[HazardyKnusperkeks]

Thanks for fixing this. It hit my quite some times, but never had the time to look into it.

[glotchimo]

Explicitly declare FormatToken type instead of using auto.

[glotchimo]

Add equality operator tests.

[curdeius]

Could you check if your patch fixes https://github.com/llvm/llvm-project/issues/33044 as well?
If so, please add tests.

[MyDeveloperDay]

In D117421#3246627, @glotchimo wrote:

I don't know why, but clang-format reformatted most if not all of the long/block comments in WhitespaceManager.cpp. Will it be necessary for me to revert the changes to those comments and skip formatting when updating the diff so as to keep it minimal?

If when editing you save (and clang-format) when you've missed a } then the file likely reindented, this, in turn, reflowed the comments, which means when you add the '}' back in, those comments have newlines in different places

once reflowed the comments don't reflow back.

This is commonly how I mess up, I then git difftool with something like winmerge and revert the comments one by one.

[MyDeveloperDay]

clang/unittests/Format/FormatTest.cpp

16172

can you add a test showing what happens when you don't have the { in there definition?

16189

can you mi

[curdeius]

With your patch I still see the bug in this case:

struct S {
  /*comment*/ S(const S &) = default;
  void f()                 = default;
  S &operator              =(S);
};

Mind the operator =.
It happens every time that operator= has only declaration (no definition body).

Another failure:

struct S {
  /*comment*/ S(const S &) = default;
  void f()                 = default;
  S &operator/**/          =(S) {}
};

As mentioned yesterday, comment between operator and =.

Note, /*comment*/ is there just to make the spacing more visible. It's unrelated to the bug otherwise.

[curdeius]

clang/lib/Format/WhitespaceManager.cpp

735

This should fix the operator/**/ = problem.

[curdeius]

clang/lib/Format/WhitespaceManager.cpp

735–742

Can't we simplify this?

738

And this should fix the declaration-only problem.

[glotchimo]

Add overload declaration look-ahead.

[glotchimo]

I was tinkering with the use of getPreviousNonComment last night before signing off and the problem that I noticed was that, though it stops the operator= from being split and aligned with previous lines, it adds a single space:

 /* long long padding */ int() = default;
 int &operator()               = default;
-int &operator/**/=();
+int &operator/**/ =();

Going to mess around with it some more today, but do you have any ideas as to why that might be happening?

[glotchimo]

In D117421#3247632, @curdeius wrote:

Could you check if your patch fixes https://github.com/llvm/llvm-project/issues/33044 as well?
If so, please add tests.

As of right now, it doesn't fix 33044. Should I investigate and see if I can get this addressed in the same patch, or should it be considered a separate bug?

[HazardyKnusperkeks]

In D117421#3249196, @glotchimo wrote:

In D117421#3247632, @curdeius wrote:

Could you check if your patch fixes https://github.com/llvm/llvm-project/issues/33044 as well?
If so, please add tests.

As of right now, it doesn't fix 33044. Should I investigate and see if I can get this addressed in the same patch, or should it be considered a separate bug?

A different one please.

[curdeius]

In D117421#3249126, @glotchimo wrote:

I was tinkering with the use of getPreviousNonComment last night before signing off and the problem that I noticed was that, though it stops the operator= from being split and aligned with previous lines, it adds a single space:

/* long long padding */ int() = default;
 int &operator()               = default;
-int &operator/**/=();
+int &operator/**/ =();

Going to mess around with it some more today, but do you have any ideas as to why that might be happening?

I don't think it's related to getPreviousNonComment, but to the fact that we add spaces after block comments.
For me it's what we expect to have.

In D117421#3249196, @glotchimo wrote:

In D117421#3247632, @curdeius wrote:

Could you check if your patch fixes https://github.com/llvm/llvm-project/issues/33044 as well?
If so, please add tests.

As of right now, it doesn't fix 33044. Should I investigate and see if I can get this addressed in the same patch, or should it be considered a separate bug?

It will be certainly a different patch, I had just thought that your patch would fix it tangentially.

[glotchimo]

Use getPreviousNonComment to account for inline comments.

[glotchimo]

clang/lib/Format/WhitespaceManager.cpp

735–742

I previously thought this would misalign other symbols succeeding operator but realize now an identifier called operator would differ from the actual keyword. Away from machine right now but will update the patch later today.

[glotchimo]

Simplify by removing look-ahead (unnecessary b/c of difference between ident and operator tokens).

[curdeius]

LGTM.
Thanks for contributing!

Let's wait a day or two before landing to let other reviewers chime in.

Do you need help landing?
If so please provide your name and email address that you'd like to be used for the commit.
Otherwise, you can request a commit access if you continue contributing (cf. https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access).

[MyDeveloperDay]

I think this LGTM

[glotchimo]

Amazing, thank you. This is my first patch so I will need help landing, but I will also apply for commit access as I intend to keep contributing. Here is my name and email for this commit: Elliott Maguire <[e-mail]>

clang/lib/Format/WhitespaceManager.cpp

AlignTokens(
         // Do not align on equal signs that are first on a line.
         if (C.NewlinesBefore > 0)
           return false;
 
         // Do not align on equal signs that are last on a line.
         if (&C != &Changes.back() && (&C + 1)->NewlinesBefore > 0)
           return false;
 
+        // Do not align operator= overloads.
+        if (C.Tok->Previous && C.Tok->Previous->is(tok::kw_operator)) {
+          auto *Next = C.Tok->Next;
+          while (Next && Next->NewlinesBefore == 0) {
+            if (Next->Tok.is(tok::l_brace))
+              return false;
+            Next = Next->Next;
+          }
+        }
+
         return C.Tok->is(tok::equal);
       },
       Changes, /*StartAt=*/0, Style.AlignConsecutiveAssignments);
 }
 
 void WhitespaceManager::alignConsecutiveBitFields() {
   if (Style.AlignConsecutiveBitFields == FormatStyle::ACS_None)
     return;

clang/unittests/Format/FormatTest.cpp

verifyFormat("int a   = 5;\n"
                Alignment);
   verifyFormat("int oneTwoThree = 123;\n"
                "int oneTwo      = 12;\n"
                "method();\n",
                Alignment);
   verifyFormat("int oneTwoThree = 123; // comment\n"
                "int oneTwo      = 12;  // comment",
                Alignment);
+  verifyFormat("int()           = default;\n"
+               "int &operator() = default;\n"
+               "int &operator=() {",
+               Alignment);
+  verifyFormat("int()           = default; // comment\n"
+               "int &operator() = default; // comment\n"
+               "int &operator=() {",
+               Alignment);
 
   // Bug 25167
   /* Uncomment when fixed
     verifyFormat("#if A\n"
                  "#else\n"
                  "int aaaaaaaa = 12;\n"
                  "#endif\n"
                  "#if B\n"
                  "#else\n"
                  "int a = 12;\n"
                  "#endif\n",
                  Alignment);
     verifyFormat("enum foo {\n"
                  "#if A\n"
                  "#else\n"
                  "  aaaaaaaa = 12;\n"
                  "#endif\n"
                  "#if B\n"
                  "#else\n"
                  "  a = 12;\n"