Page MenuHomePhabricator

[clang-format] Adds a formatter for aligning trailing comments over empty lines
ClosedPublic

Authored by yusuke-kadowaki on Aug 18 2022, 6:06 AM.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
yusuke-kadowaki added inline comments.Sep 5 2022, 6:07 AM
clang/include/clang/Format/Format.h
298

Could you please be more specific about why you think so when pointing out something? I don't see why you think it's wrong. I also would like to mention that I followed @HazardyKnusperkeks's request on this.

My personal preference is a new struct without Enabled

clang/lib/Format/Format.cpp
1247

Won’t you need to initialise the members here as above

clang/unittests/Format/FormatTestComments.cpp
2908

Any reason why these are not verifyformats.

MyDeveloperDay added inline comments.Sep 5 2022, 6:20 AM
clang/include/clang/Format/Format.h
298

Ignore my comment

446

Ignore this comment

yusuke-kadowaki marked 6 inline comments as done.
  • Change to use verifyFormat
clang/lib/Format/Format.cpp
1247

Members are false when initialized so I don't know why those above are initialized explicitly as false.

MyDeveloperDay added inline comments.Sep 5 2022, 10:21 AM
clang/unittests/Format/FormatTestComments.cpp
2930

Why not verifyFormat here too and below?

My personal preference is a new struct without Enabled

We go for a new struct. But why without enabled?
Currently we have a boolean on/off switch AlignTrailingComments. This change wanted to add a second boolean to enable aligning ignoring empty lines, naturally its value only matters if AlignTrailingComments is true.
My request was to use the existing struct (and I'm still in favor of that, but not strong enough to fight anything or anybody over it), but a struct seems to be better than 2 booleans. But an on/off switch still needs to be there, and at least using the same name as other options Enabled seems to be the most consistent solution.

My personal preference is a new struct without Enabled

We go for a new struct. But why without enabled?
Currently we have a boolean on/off switch AlignTrailingComments. This change wanted to add a second boolean to enable aligning ignoring empty lines, naturally its value only matters if AlignTrailingComments is true.
My request was to use the existing struct (and I'm still in favor of that, but not strong enough to fight anything or anybody over it), but a struct seems to be better than 2 booleans. But an on/off switch still needs to be there, and at least using the same name as other options Enabled seems to be the most consistent solution.

We don’t normally use Enabled for other structs, maybe we don’t use bool but enums (Always, Never, Leave)

yusuke-kadowaki marked 7 inline comments as done.
  • Introduce new struct
  • Update document

This change should be ok for everyone (I hope). BTW, please correct my English if any or create a whole new one for me since I'm not so sure what I wrote delivers well.

yusuke-kadowaki added inline comments.Sep 6 2022, 8:08 AM
clang/unittests/Format/FormatTestComments.cpp
2930

To test the ColumnLimit behavior.

MyDeveloperDay added inline comments.Sep 6 2022, 12:50 PM
clang/unittests/Format/FormatTestComments.cpp
2930

pretty sure you can pass in a style

MyDeveloperDay added inline comments.Sep 6 2022, 12:59 PM
clang/docs/ClangFormatStyleOptions.rst
931

Is Don'tAlign the same as Leave that other options support (for options it best if we try and use the same terminiology

Always,Never,Leave (if that fits)

clang/include/clang/Format/Format.h
373

all options have a life cycle

bool -> enum -> struct

One of the thing I ask you before, would we want to align across N empty lines, if ever so they I think
we should go straight to a struct

yusuke-kadowaki marked 3 inline comments as done.Sep 7 2022, 5:48 AM
yusuke-kadowaki added inline comments.
clang/docs/ClangFormatStyleOptions.rst
931

IMHO, Leave probably fits but DontAlign is more straightforward. Also DontAlign is used for other alignment styles like BracketAlignmentStyle so it would be more natural.

clang/include/clang/Format/Format.h
373

all options have a life cycle

I see your point. But we are checking Style.MaxEmptyLinesToKeep to determine how many consecutive lines to align. So currently no need to specify it from the option. You'll find the implementation below.

clang/unittests/Format/FormatTestComments.cpp
2930

Style isn't a problem here. But when testing column limit, you want to see before and after the line split into multiple lines. Original test cases of AlignTrailingComments also use this EXPECT_EQ style not verifyFormat.

MyDeveloperDay added inline comments.Sep 7 2022, 12:53 PM
clang/docs/ClangFormatStyleOptions.rst
931

Leave should mean do nothing.. I'm personally not a fan of DontAlign because obvious there should be a ' between the n and the t but I see we use it elsewhere

But actually now I see your DontAlign is effectively the as before (i.e. it will not add extra spaces)

The point of Leave is what people have been asking for when we introduce a new option, that is we if possible add an option which means "Don't touch it at all" i.e. if I want to have

int a;           //  abc
int b;   /// bcd
int c;                                    // cde

Then so be it

clang/include/clang/Format/Format.h
373

I think its a bad idea to hijack a different option to do this..I don't think it means the same thing and I think what whilst it might work there will be someone somewhere who doesn't want it to behave like this and will call it out as a bug.

clang/docs/ClangFormatStyleOptions.rst
931

Leave is a nice option, yes.
I think it is complementary to Dont.

But maybe if the option is called AlignTrailingComments one may interpret Leave not as "don't touch the position of a comment at all" (what do we do, if the comment is outside of the column limit?), but as "just don't touch them, when they are somewhat aligned". Just a thought.

clang/include/clang/Format/Format.h
373

Since you are strictly against Enabled what to put into a struct?

an example of the exact reason why we should not reuse the same struct... https://github.com/llvm/llvm-project/issues/57464

another reason for using a struct, is that we might want to say something like "AlignNamespaceTrainingComments:false" https://github.com/llvm/llvm-project/issues/57504

yusuke-kadowaki marked 7 inline comments as done.Sep 8 2022, 8:40 AM
yusuke-kadowaki added inline comments.
clang/docs/ClangFormatStyleOptions.rst
931

Leave should mean do nothing

Ok now I see what Leave means.
But that should be another piece of work no?

Of course me or someone can add the feature later (I don't really know how to implement that though at least for now)

clang/include/clang/Format/Format.h
373

Since you are strictly against Enabled what to put into a struct?

@MyDeveloperDay
Can you answer this? Plus it would be helpful if you just write down what your ideal struct be like.

I don't think it means the same thing

How so? There's no empty lines more than MaxEmptyLinesToKeep all the time no?

an example of the exact reason why we should not reuse the same struct... https://github.com/llvm/llvm-project/issues/57464

Just a bit rephrasing.

another reason for using a struct, is that we might want to say something like "AlignNamespaceTrainingComments:false" https://github.com/llvm/llvm-project/issues/57504

Something like that we really want, and that is something I'd like to put on top of this change. Also for all comments following a }. That's the only reason I've disabled aligning comments for now.

clang/docs/ClangFormatStyleOptions.rst
931

Fine by me.

clang/include/clang/Format/Format.h
373

I don't think it means the same thing

How so? There's no empty lines more than MaxEmptyLinesToKeep all the time no?

But you could set MaxEmptyLinesToKeep to 3 and aligning comments to over 2 empty lines.

int thisIsAVariable = 5; // It Really is


int stillAligned = 2;    // Yep



int notSoMuch = 2; // Nope

It certainly doesn't hurt to have a value there.

373

Since you are strictly against Enabled what to put into a struct?

@MyDeveloperDay
Can you answer this? Plus it would be helpful if you just write down what your ideal struct be like.

For me it would currently look like:

struct Style {
  enum E {
    Yes,
    No,
  };
  E State;
  unsigned OverEmptyLines;
}

Names are open for debate.

struct Style {
  enum E {
    Yes,
    No,
  };
  unsigned OverEmptyLines;
}

I don't understand the need for state as a struct could have multiple options (as enums) each enum should have a state that means "Leave"

But you could set MaxEmptyLinesToKeep to 3 and aligning comments to over 2 empty lines.

Correct! lets assume MaxEmptyLinesToKeep = 3

if this case is valid in that case

int a;            // Foo
int longer foo;   // Foo
int verylong foo; // Foo

then so is this

int a;            // Foo

int longer foo;   // Foo

int verylong foo; // Foo

and so is this...

int a;            // Foo


int longer foo;   // Foo


int verylong foo; // Foo

but not this

int a;          // Foo

int longer foo; // Foo


 
int verylong foo; // Foo

Regardless of how many lines I am willing or not willing to keep, I know it feels orthogonal, but its actually independent (so don't use it as my setting), or you could manipulate code in a way I don't want changed (and they people will complain)

Its one of those cases there it feels like they can be the same, but the answer is should they? or are we making an assumption about what people want rather than giving them the control.

I agree if OverEmptyLines > MaxEmptyLinesToKeep then it feels kinda odd (but what about lines that are // clang-format off'd should we count those?

yusuke-kadowaki marked an inline comment as done.

Just updated the Style struct field definitions for review. Haven't implemented the logics.

yusuke-kadowaki marked 2 inline comments as done.Sep 10 2022, 10:14 AM

Thank you for the detailed explanation. I understood the needs for unsigned OverEmptyLines field.
Please review the struct definition first. Then I'll implement the rest of the code.

clang/include/clang/Format/Format.h
428–433

I don't understand the need for state as a struct could have multiple options (as enums) each enum should have a state that means "Leave"

@MyDeveloperDay
Without having state, how can this be implemented?

MyDeveloperDay added inline comments.Sep 12 2022, 9:40 AM
clang/include/clang/Format/Format.h
406

Kind? doesn't feel like the right word

428–433

bool Enabled = (Kind != FormatStyle::TCAS_Leave)

yusuke-kadowaki marked 2 inline comments as done.Sep 12 2022, 10:03 AM

So other than the naming, does the struct look good?

clang/include/clang/Format/Format.h
406

What do you recommend?

428–433

Oh ok so I think we are on the same page.

MyDeveloperDay added inline comments.Sep 14 2022, 1:51 AM
clang/include/clang/Format/Format.h
428

do you need this?

yusuke-kadowaki marked 3 inline comments as done.

Implementation done except for the Leave option.

clang/include/clang/Format/Format.h
428

Yes. Without this it does not compile.

clang/docs/ClangFormatStyleOptions.rst
931

@HazardyKnusperkeks
Is this complicated? If it's not I might as well do this.

Also it would be helpful if you could provide some implementation guidance. Sorry to ask this even though I haven't tried it myself yet.

clang/docs/ClangFormatStyleOptions.rst
931

@HazardyKnusperkeks
Is this complicated? If it's not I might as well do this.

Also it would be helpful if you could provide some implementation guidance. Sorry to ask this even though I haven't tried it myself yet.

I think you refer to the non aligning of comments following r_braces. I don't think so, because at least the cases I think about the r_brace should be the first token on a unwrapped line, so you just have to check for Line->First.

Implment trailing comments Leave option

There is two options, I think, when leaving comments exceeds the column limit.

  1. Just format the trailing comments
  2. Ignore the column limit for that line.

This implementation does the first option because ignoring column limit semms too much for just taking care of trailing comments.

yusuke-kadowaki marked an inline comment as done.Oct 10 2022, 7:47 AM
yusuke-kadowaki added inline comments.
clang/docs/ClangFormatStyleOptions.rst
931

Isn't this for https://github.com/llvm/llvm-project/issues/57504?
I thought that's different than leaving all the trailing comments.

clang/lib/Format/WhitespaceManager.cpp
967–969

Elide the braces.

clang/unittests/Format/FormatTestComments.cpp
3045

Can you do that in reverse order (more spaces at top) also?

yusuke-kadowaki marked 3 inline comments as done.

Added more tests
Removed braces

For me this is good. But please wait for @MyDeveloperDay .

clang/lib/Format/WhitespaceManager.cpp
949–951

These braces also.

Can you mark your comments done, I think you missed removing some braces that @HazardyKnusperkeks asked you to remove

I think you also need to rebase to get rid of the "ClangFormatStyleOptions.rst" (which means you need to rebase and rerun dump_format_style.py

MyDeveloperDay requested changes to this revision.Oct 14 2022, 9:20 AM
This revision now requires changes to proceed.Oct 14 2022, 9:20 AM
yusuke-kadowaki marked 2 inline comments as done.
  • Rebased
  • Remove more braces
  • Update rst
MyDeveloperDay accepted this revision.Oct 17 2022, 2:53 PM
This revision is now accepted and ready to land.Oct 17 2022, 2:53 PM

Thank you for all the reviews. Appreciate it.

So will some of you land this or am I supposed to do something else?

rymiel added a subscriber: rymiel.Oct 18 2022, 6:14 AM

Please provide a name and an email so someone could commit it on your behalf

Please provide a name and an email so someone could commit it on your behalf

name: Yusuke Kadowaki
email: yusuke.kadowaki.1231@gmail.com

@MyDeveloperDay @HazardyKnusperkeks

Could you land this please if we are not waiting for anything?

This revision was landed with ongoing or failed builds.Oct 30 2022, 5:23 AM
This revision was automatically updated to reflect the committed changes.