This patch addresses https://github.com/llvm/llvm-project/issues/19756
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang/include/clang/Format/Format.h | ||
---|---|---|
386 | Maybe you can port this to AlignConsecutiveStyle? AcrossComments would not affect anything in this context. | |
388 | Documentation is missing. | |
clang/lib/Format/Format.cpp | ||
1455 | Not needed since it inherits everything from LLVMStyle. | |
clang/lib/Format/WhitespaceManager.cpp | ||
930 | Just make it clear for everyone, so one would not need to guess, when trying to understand the code. |
- 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.
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. | |
386 | Take a look at D127578. | |
clang/lib/Format/Format.cpp | ||
637 | Please sort. |
clang/lib/Format/Format.cpp | ||
---|---|---|
649 | you can't remove an option, otherwise you'll break everyones .clang-format |
clang/unittests/Format/FormatTest.cpp | ||
---|---|---|
20044 | don't remove |
clang/lib/Format/Format.cpp | ||
---|---|---|
649 | 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 |
clang/docs/ClangFormatStyleOptions.rst | ||
---|---|---|
797 | why do we need Enabled? isn't it just false: AcrossEmptyLines: false AcrossComments: false true: AcrossEmptyLines: true AcrossComments: true |
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 | ||
649 | Do you disagree that it actually is parsed? But that compatibility parsing has nothing to do with ignoring unknown (new) options. |
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? |
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, 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 | ||
649 | 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 |
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. |
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. |
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,
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 | ||
---|---|---|
3688 | Anyone knows where this comes from? | |
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 | ||
930 | And accompanied by a short test. That should be everything to support the mixture of the options, right? | |
988 | Is this clang-format formatted? | |
clang/unittests/Format/FormatTestComments.cpp | ||
4277 | Please revert this. |
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.
- 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 | ||
---|---|---|
3688 | 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 | ||
930 |
I think so. Done. | |
988 | I forgot that. Also formatted some other diffs. | |
clang/unittests/Format/FormatTestComments.cpp | ||
4277 | 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? | |
384 | And this |
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. |
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. |
- Change to use verifyFormat
clang/lib/Format/Format.cpp | ||
---|---|---|
1196 | Members are false when initialized so I don't know why those above are initialized explicitly as false. |
clang/unittests/Format/FormatTestComments.cpp | ||
---|---|---|
2930 | Why not verifyFormat here too and below? |
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)
- 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.
clang/unittests/Format/FormatTestComments.cpp | ||
---|---|---|
2930 | To test the ColumnLimit behavior. |
clang/unittests/Format/FormatTestComments.cpp | ||
---|---|---|
2930 | pretty sure you can pass in a style |
clang/docs/ClangFormatStyleOptions.rst | ||
---|---|---|
978 | 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 | ||
385 | 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 |
clang/docs/ClangFormatStyleOptions.rst | ||
---|---|---|
978 | 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 | ||
385 |
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. |
clang/docs/ClangFormatStyleOptions.rst | ||
---|---|---|
978 | 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 | ||
385 | 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 | ||
---|---|---|
978 | Leave is a nice option, yes. 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 | ||
385 | 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
clang/docs/ClangFormatStyleOptions.rst | ||
---|---|---|
978 |
Ok now I see what Leave means. 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 | ||
385 |
@MyDeveloperDay
How so? There's no empty lines more than MaxEmptyLinesToKeep all the time no? |
Just a bit rephrasing.
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 | ||
---|---|---|
978 | Fine by me. | |
clang/include/clang/Format/Format.h | ||
385 |
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. | |
385 |
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?
Just updated the Style struct field definitions for review. Haven't implemented the logics.
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 | ||
---|---|---|
440–445 |
@MyDeveloperDay |
clang/include/clang/Format/Format.h | ||
---|---|---|
440 | do you need this? |
Implementation done except for the Leave option.
clang/include/clang/Format/Format.h | ||
---|---|---|
440 | Yes. Without this it does not compile. |
clang/docs/ClangFormatStyleOptions.rst | ||
---|---|---|
978 | @HazardyKnusperkeks 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 | ||
---|---|---|
978 |
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.
- Just format the trailing comments
- 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.
clang/docs/ClangFormatStyleOptions.rst | ||
---|---|---|
978 | Isn't this for https://github.com/llvm/llvm-project/issues/57504? |
For me this is good. But please wait for @MyDeveloperDay .
clang/lib/Format/WhitespaceManager.cpp | ||
---|---|---|
931–933 | 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
Thank you for all the reviews. Appreciate it.
So will some of you land this or am I supposed to do something else?
@MyDeveloperDay @HazardyKnusperkeks
Could you land this please if we are not waiting for anything?
may be AcrossEmptyLines should be a number (to mean the number of empty lines)