Page MenuHomePhabricator

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

Authored by glotchimo on Jan 16 2022, 12:36 AM.

Diff Detail

Event Timeline

glotchimo created this revision.Jan 16 2022, 12:36 AM

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 updated this revision to Diff 400390.Jan 16 2022, 9:13 AM

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

glotchimo published this revision for review.Jan 16 2022, 9:19 AM
glotchimo added reviewers: MyDeveloperDay, djasper.
glotchimo added a project: Restricted Project.
glotchimo added a subscriber: cfe-commits.
Herald added a project: Restricted Project. · View Herald TranscriptJan 16 2022, 9:19 AM
curdeius retitled this revision from Fix 31568 (i.e. incorrect alignment of operator= overloads). to [clang-format] Fix incorrect alignment of operator= overloads..Jan 16 2022, 10:00 AM
curdeius edited the summary of this revision. (Show Details)
curdeius edited reviewers, added: curdeius, owenpan, HazardyKnusperkeks; removed: djasper.

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
16271

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

16273

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

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

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

Explicitly declare FormatToken type instead of using auto.

glotchimo marked an inline comment as done.

Add equality operator tests.

glotchimo marked an inline comment as done.Jan 17 2022, 12:21 AM

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

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.

clang/unittests/Format/FormatTest.cpp
16271

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

16288

can you mi

curdeius added a comment.EditedJan 17 2022, 12:52 AM

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 added inline comments.Jan 17 2022, 12:58 AM
clang/lib/Format/WhitespaceManager.cpp
735

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

curdeius added inline comments.Jan 17 2022, 1:04 AM
clang/lib/Format/WhitespaceManager.cpp
735–742

Can't we simplify this?

738

And this should fix the declaration-only problem.

glotchimo marked an inline comment as done.

Add overload declaration look-ahead.

glotchimo added a comment.EditedJan 17 2022, 10:32 AM

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?

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?

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.

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.

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 marked 4 inline comments as done.

Use getPreviousNonComment to account for inline comments.

glotchimo added inline comments.Jan 17 2022, 2:39 PM
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 updated this revision to Diff 400696.EditedJan 17 2022, 7:34 PM
glotchimo marked an inline comment as done.

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

glotchimo edited the summary of this revision. (Show Details)Jan 17 2022, 9:19 PM
curdeius accepted this revision.Jan 17 2022, 11:44 PM

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).

This revision is now accepted and ready to land.Jan 17 2022, 11:44 PM
MyDeveloperDay accepted this revision.Jan 18 2022, 1:06 AM

I think this LGTM

owenpan accepted this revision.Jan 18 2022, 1:26 AM

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 <glotchimo@pm.me>

This revision was landed with ongoing or failed builds.Jan 19 2022, 1:19 AM
This revision was automatically updated to reflect the committed changes.