This is an archive of the discontinued LLVM Phabricator instance.

[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
HazardyKnusperkeks added a project: Restricted Project.Aug 19 2022, 12:24 PM
clang/include/clang/Format/Format.h
448

Maybe you can port this to AlignConsecutiveStyle? AcrossComments would not affect anything in this context.

450

Documentation is missing.

clang/lib/Format/Format.cpp
1510

Not needed since it inherits everything from LLVMStyle.
Same for the other styles.

clang/lib/Format/WhitespaceManager.cpp
948

Just make it clear for everyone, so one would not need to guess, when trying to understand the code.

clang/lib/Format/Format.cpp
1659

ditto remove

clang/unittests/Format/FormatTest.cpp
25894

I think it needs more tests than just include examples

yusuke-kadowaki updated this revision to Diff 454864.EditedAug 23 2022, 8:51 AM
yusuke-kadowaki edited the summary of this revision. (Show Details)
  • Port AlignTrailingComments to one of the members of AlignConsecutiveStyle
  • Add documentation
  • Move tests to FormatTestComments
  • Add more tests
    • I left some failing tests that I think we should add implementations to make them pass. That's because I wanted to get some reviews for the work I've done so far before tackling on it.
clang/include/clang/Format/Format.h
448

I took this approach

clang/unittests/Format/FormatTest.cpp
74–77

Sorry I will remove these next time I update this patch.

Please mark things as done.

clang/include/clang/Format/Format.h
297

Run clang/docs/tools/dump_format_style.py.

306

16

I'm pretty sure we don't need longer than that. :)

But mention that it existed as AlignTrailingComments since 3.7. There should be examples for that in this file.

448

Take a look at D127578.

clang/lib/Format/Format.cpp
685

Please sort.

yusuke-kadowaki marked 9 inline comments as done.
  • Remove my comments
  • Update documentation
  • Sort
MyDeveloperDay requested changes to this revision.Aug 25 2022, 7:50 AM
MyDeveloperDay added inline comments.
clang/lib/Format/Format.cpp
658

you can't remove an option, otherwise you'll break everyones .clang-format

This revision now requires changes to proceed.Aug 25 2022, 7:50 AM
MyDeveloperDay added inline comments.Aug 25 2022, 7:52 AM
clang/unittests/Format/FormatTest.cpp
20314

don't remove

clang/lib/Format/Format.cpp
658

That's not correct. We have done it:
D108752 -> D108882 -> D127390

You can remove (and in this case should), but you still need to parse it and act accordingly. Which is done as far as I can see.

clang/unittests/Format/FormatTest.cpp
20314

It's tested below.

MyDeveloperDay accepted this revision.Aug 26 2022, 2:23 AM
MyDeveloperDay added inline comments.
clang/lib/Format/Format.cpp
658

sorry thats what I meant, you can remove it, but you have to make it turn on the correct new style that keeps exactly the old user case, and you can't remove it from the configuration parsing otherwise anyone who has it in their .clang-format is immediately broken with an unknown option.

to be honest this is an annoyance for introducing new features, which at some point I'd like to drop, you can't have a new option which is not read

For me, when we introduced new languages, or new features like InsertBraces etc.. I want to put it in my .clang-format even though other people they aren't using a version that uses it. (becuase it won't impact them), i.e. it won't add braces or correct QualifierOrder that doesn't bother me

This revision is now accepted and ready to land.Aug 26 2022, 2:23 AM
MyDeveloperDay added inline comments.Aug 26 2022, 2:25 AM
clang/docs/ClangFormatStyleOptions.rst
797

why do we need Enabled?

isn't it just

false:
AcrossEmptyLines: false
AcrossComments: false

true:
AcrossEmptyLines: true
AcrossComments: true
MyDeveloperDay requested changes to this revision.Aug 26 2022, 2:26 AM
This revision now requires changes to proceed.Aug 26 2022, 2:26 AM
clang/docs/ClangFormatStyleOptions.rst
797

The struct is a bit older. And we need Enabled, one might wish to align only really consecutive comments, as the option right now does. (Same for macros, assignments, etc..)

clang/lib/Format/Format.cpp
658

Do you disagree that it actually is parsed?

But that compatibility parsing has nothing to do with ignoring unknown (new) options.

MyDeveloperDay added inline comments.Aug 29 2022, 2:16 PM
clang/docs/ClangFormatStyleOptions.rst
785

may be AcrossEmptyLines should be a number (to mean the number of empty lines)

794

Should this say AlignedConsecutuveCommets

797

I'm still not sure I understand, plus are those use cases you describe tested, I couldn't see that.

If feels like what you are saying is that AlignConsecutiveStyle is used elsewhere and it has options that don't pertain to aligning comments?

I couldn't tell if feel like this documentation is describing something other than AligningTrailingComments, if I'm confused users could be too?

clang/unittests/Format/FormatTestComments.cpp
2876

can we have real comments with different lengths?

yusuke-kadowaki marked 9 inline comments as done.
  • Fix the newline condition
  • Update tests
clang/docs/ClangFormatStyleOptions.rst
785

We need to introduce a new struct to do that since AlignConsecutiveStyle is shared with some options and not possible to be changed. Plus I think more than two empty lines are formatted to one empty line anyway.

794

No. This part is a documentation about AlignConsecutiveStyle type, which is appended automatically after all the options that take AlignConsecutiveStyle type.

797

As for the Enabled option,
Enabled: true
AcrossEmptyLines: false

is supposed to work in the same way as the old style AlignTrailingComments. So the tests of AlignTrailingComments are the test cases.

For the documentation, I also thought it was a bit confusing when I first saw it because the description of the option and the style is kind of connected. But as I also mentioned above, this part is automatically append and it affects all the other options that have AlignConsecutiveStyle if we change. So I think it should be another patch even if we are to modify it.

clang/lib/Format/Format.cpp
658

I confirmed the old style AlignTrailingComments works and it's also tested with CHECK_PARSE if I understand correctly.

clang/docs/ClangFormatStyleOptions.rst
785

There MaxEmptyLinesToKeep which would allow to set it higher.

If we want to change the bool to an unsigned, then that should be a different change.

794

Which we could change to reflect that it's used for multiple options.

797

I'm still not sure I understand, plus are those use cases you describe tested, I couldn't see that.

If feels like what you are saying is that AlignConsecutiveStyle is used elsewhere and it has options that don't pertain to aligning comments?

I couldn't tell if feel like this documentation is describing something other than AligningTrailingComments, if I'm confused users could be too?

It was added in D93986 as an enum, and in D119599 made into a struct which then had 2 options only valid for assignments, not the macros or declarations. Both accepted by you.

So I see no problem, but think it is the right thing, to port aligning comments to the struct, even if the flag AccrossComments is a noop in this case. When this lands I actually plan to add a flag only used by comments.

clang/unittests/Format/FormatTestComments.cpp
2863

Interesting would be a comment which is split, do we continue to align, or not?

2958

Trailing whitespace.

yusuke-kadowaki marked 4 inline comments as done.
  • Remove trailing whitespace
  • Update document
clang/docs/ClangFormatStyleOptions.rst
785

Oh I didn't know MaxEmptyLinesToKeep. Thank you for sharing.
I can try to change it that way when we done with this patch. Test cases would be complicated tho.

clang/unittests/Format/FormatTestComments.cpp
2863

Could you give me a specific example?

clang/include/clang/Format/Format.h
143

That I wouldn't change.

153

The change/addition has to be here, since here it directly states AlignConsecutiveMacros.

clang/unittests/Format/FormatTestComments.cpp
2863

Something like

int foo = 2323234; // Comment
int bar = 52323;   // This is a very long comment, ...
                   // which is wrapped around.

int x = 2; // Is this still aligned?

You may need to make the comment longer or reduce the column limit, as often used for testing the wrapping behavior.

  • Update tests
  • Fix document
yusuke-kadowaki marked an inline comment as done.Sep 1 2022, 9:07 AM
yusuke-kadowaki added inline comments.
clang/include/clang/Format/Format.h
143

reverted

153

What are you meaning to say here? I thought saying AlignConsecutiveStyle is used for multiple options is what we are trying to do.

Should we say something like,

Here AlignConsecutiveMacros is used as an example, but in practice AlignConsecutiveStyle is also used with other options.

in this place?

clang/unittests/Format/FormatTestComments.cpp
2863

Added that case with some column limits. I think it's working as expected. What do you think?

In my opinion we are nearly done.

clang/docs/ClangFormatStyleOptions.rst
3658

Anyone knows where this comes from?
You are using the script to generate the rst, right?

clang/include/clang/Format/Format.h
153

The example mentions explicitly only AlignConsecutiveMacros which is a bit misleading if you are only looking at the documentation of AlignConsecutiveAssignments for example.

This is not your fault, and I'm fine with ignoring it here. A separate patch to fix that is welcome. :)

clang/lib/Format/WhitespaceManager.cpp
948

And accompanied by a short test. That should be everything to support the mixture of the options, right?

1020

Is this clang-format formatted?

clang/unittests/Format/FormatTestComments.cpp
4319

Please revert this.

Is there a technical reason for reusing the struct rather than introducing a new one?

Is there a technical reason for reusing the struct rather than introducing a new one?

I see, good point. Really only if one would port the implementation to AlignTokens. If that's feasible (or sensible) I don't know. Otherwise one will only reuse the parsing code.

yusuke-kadowaki marked 8 inline comments as done.
  • Revert doc
  • Revert rst as well
  • Apply format
  • Update implementation to deal with the setting of MaxEmptyLinesToKeep
  • Add test for the combination with MaxEmptyLinesToKeep
clang/docs/ClangFormatStyleOptions.rst
3658

Yes, I'm using dump_format_style.py.

clang/include/clang/Format/Format.h
153

Got it. Reverted for now.

clang/lib/Format/WhitespaceManager.cpp
948

That should be everything to support the mixture of the options, right?

I think so.

Done.

1020

I forgot that. Also formatted some other diffs.

clang/unittests/Format/FormatTestComments.cpp
4319

Reverted.

I feel this patch leave the documentation in a right state, I won’t be giving it an accept in this form. Please also mark the comments as done once addressed so we know you’ve read and fixed our requests

clang/docs/ClangFormatStyleOptions.rst
823

I’m sorry I just don’t think this documentation is now correct, all because you are trying I believe to reuse a structure that has options that’s not needed,

clang/include/clang/Format/Format.h
298

Isn’t this completely wrong?

446

And this

MyDeveloperDay added inline comments.Sep 5 2022, 3:50 AM
clang/docs/ClangFormatStyleOptions.rst
794

Or you could use your own struct! And it would be correct and not full of options that don’t apply.

yusuke-kadowaki marked an inline comment as done.Sep 5 2022, 6:07 AM

Can we all agree with the decision whether we use the AlignConsecutiveStyle or introduce a new struct before I start implementing or updating the document?
IMO, both are reasonable in some respects so I'd like you owners to decide.

Please also mark the comments as done once addressed so we know you’ve read and fixed our requests

I'm reading all the comments but leaving some comments not done on purpose that I think the discussion is not done. Isn't it a right thing to do here?

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.