This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Add option to align compound assignments like `+=`
ClosedPublic

Authored by sstwcw on Feb 11 2022, 3:06 PM.

Diff Detail

Event Timeline

sstwcw requested review of this revision.Feb 11 2022, 3:06 PM
sstwcw created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 11 2022, 3:06 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
curdeius requested changes to this revision.Feb 12 2022, 12:51 AM

Thanks for working on this!
Looks pretty good already.

clang/include/clang/Format/Format.h
253

You need to update the RST files when updating the doc comments here. Please use https://github.com/llvm/llvm-project/blob/main/clang/docs/tools/dump_format_style.py for that.

259

I guess it would be complicated to avoid adding an additional space here. I mean, it could be:

a  &= 2;
bbb = 2;

And with 3-char operators, there's one more space.

clang/unittests/Format/FormatTest.cpp
16783

Please test spacing in separate verifyFormat cases as in the Format.h option description.

16792

Please test shift operators <<= and >>=.

17351

Is it something that can be done/fixed separately?

This revision now requires changes to proceed.Feb 12 2022, 12:51 AM
MyDeveloperDay added inline comments.
clang/include/clang/Format/Format.h
265

This option is not independent of AlignConsecutiveAssignments is it? this will cause confusion when people turn it on without turning on AlignConsecutiveAssignments

Options have a lifecycle we have noticed

  1. They start as bool
  2. They become enums
  3. They then end up as a struct of bool
  4. Sometimes the struct becomes of enums

Whilst I like what you are doing here I fear we will get bugs where people say I set AlignCompoundAssignments: true but its doesn't work.

AlignConsecutiveAssignments is already gone too far on the enum, it should be a struct

so rather than

enum AlignConsecutiveStyle {
    ACS_None,
    ACS_Consecutive,
    ACS_AcrossEmptyLines,
    ACS_AcrossComments,
    ACS_AcrossEmptyLinesAndComments
  };
AlignConsecutiveStyle  AlignConsecutiveAssignments ;

it should be perhaps

struct {
    bool AcrossEmptyLines,
    bool AcrossComments,
    bool AlignCompound
} AlignConsecutiveStyle;

AlignConsecutiveStyle  AlignConsecutiveAssignments;

in the .clang-format file it would become

AlignConsecutiveAssignments: Custom
AlignConsecutiveAssignmentsOptions:
           AcrossEmptyLines: true
           AcrossComments: true
           AlignCompound: false

I realise this would be a much bigger piece of work (in the config) but the existing options would map to the new options, and then we have a structure for which have space for future expansion.

The introduction of a dependent option in my view triggers the need for that config change? @HazardyKnusperkeks
you thoughts, I know we've done this before, what do you think in this case?

MyDeveloperDay added inline comments.Feb 12 2022, 2:05 AM
clang/lib/Format/WhitespaceManager.cpp
471

whats an anchor?

585–594

I like the final outcome, I'm uneasy about renaming all the variables, just because you now understand them. I'm struggling to read the algorithm in the same context as the prior version

clang/unittests/Format/FormatTest.cpp
16783

I sort of feel this isn't enough tests

17232

Huge no from me, don't change the tests because they break based on your change.

19746

alphabetic, but see my note as you shouldn't need this?

19749

IJKL last L looked i came before l right? so why did you move this down

MyDeveloperDay requested changes to this revision.Feb 12 2022, 2:05 AM
clang/include/clang/Format/Format.h
259

That would be awesome, but it should be an option to turn off or on.
But I think this would really be complicated.

265

I would even go further (and that I already told the last time). Drop the `Custom` and map the old enums to the struct when parsing, so no new option.

sstwcw marked 6 inline comments as done.Feb 12 2022, 4:22 AM
sstwcw added inline comments.
clang/include/clang/Format/Format.h
253

I guess it is saying I need to say what version the option first appeared in. How do I know what version it will be?

259

I can do it either way. But I thought without the extra space the formatted code looked ugly, especially when mixing >>= and =. Which way do you prefer?

a >>= 2;
bbb = 2;
clang/lib/Format/WhitespaceManager.cpp
585–594

Originally we tracked the column to put the operators to, so ChangeMinColumn and ChangeMaxColumn made sense. Now the column we are tracking is no longer the column to which we change the start of the operators to, the original names don't make sense any more.

clang/unittests/Format/FormatTest.cpp
17232

Here is the problem in isolation: https://reviews.llvm.org/D119625. What do you suggest about this test?

17351

I added a revision here https://reviews.llvm.org/D119625. If you commit it separately there will be a merge conflict though.

19749

Sorry. I forgot my ABC's.

sstwcw updated this revision to Diff 408172.Feb 12 2022, 4:29 AM
sstwcw marked an inline comment as done.
sstwcw marked 5 inline comments as not done.
MyDeveloperDay requested changes to this revision.Feb 12 2022, 4:55 AM
MyDeveloperDay added inline comments.
clang/lib/Format/WhitespaceManager.cpp
276

I guess I just don't understand what we mean by AnchorWidth

clang/unittests/Format/FormatTest.cpp
16836

Is there a reason you couldn't use verifyFormat here? I would expect it to messUp the formatting and put it back correctly no?

This revision now requires changes to proceed.Feb 12 2022, 4:55 AM
sstwcw updated this revision to Diff 408177.Feb 12 2022, 5:51 AM
sstwcw added inline comments.Feb 12 2022, 5:55 AM
clang/lib/Format/WhitespaceManager.cpp
276

Now that I look at it again it seems like only one column parameter is needed.

clang/unittests/Format/FormatTest.cpp
16836

messUp turns every newline into a space.

clang/include/clang/Format/Format.h
253

The next major, currently 15.
If it would take to long to review we change the version in between. But since the branching for 14 has quite recently happened this is either going in 15, or not at all. :)

259

I would prefer an option, to be able to do both.
I think I would use the variant which is more compact, but can't say for sure until I really have the option and tried it.

HazardyKnusperkeks removed a project: Restricted Project.Feb 12 2022, 9:24 AM
sstwcw added inline comments.Feb 12 2022, 2:08 PM
clang/include/clang/Format/Format.h
259

You mean like a new entry under AlignConsecutiveStyle with the options to align the left ends of the operators and to align the equal sign with and without padding them to the same length? I don't want the new option to be merged with AlignCompound, because we are working on adding support for a language that uses both = and <= for regular assignments, and maybe someone will want to support a language that has both = and := in the future.

clang/include/clang/Format/Format.h
259

Yeah. Basing on @MyDeveloperDay 's suggestion:

struct AlignConsecutiveStyle {
  bool Enabled;
  bool AcrossComments;
  bool AcrossEmptyLines;
  bool AlignCompound;
  bool CompactWhitespace; // This is just a suggestion, I'm open for better names.
};
sstwcw updated this revision to Diff 408295.Feb 13 2022, 2:14 PM
sstwcw edited the summary of this revision. (Show Details)
sstwcw added inline comments.Feb 13 2022, 2:29 PM
clang/include/clang/Format/Format.h
259

I added the new option. Moving the old options into the struct turned out to be too much work for me on a weekend. If you can accept the configuration style in the current revision, I will probably move the old options into the struct at some later date. By the way, what is the purpose of FormatStyle::operator==? It looks like it does not compare`BraceWrapping`.

clang/unittests/Format/FormatTest.cpp
17232

I hadn't noticed that column limit = 0 meant no limit. This test doesn't really need to change.

sstwcw marked 5 inline comments as done.Feb 13 2022, 2:33 PM
sstwcw added inline comments.
clang/unittests/Format/FormatTest.cpp
17351

I hadn't noticed that column limit = 0 meant no limit. This test doesn't really need to change.

I think we are going a very good way. Just some steps ahead.

clang/include/clang/Format/Format.h
259

I can believe that it is too much for a weekend. But I'd like the complete migration in one patch. There are also other align styles, which currently use the same enum. And they should keep to use the same type (struct in this case). Even though there are then some options which are not (yet?) applicable to them.

We can mark that in the code:

Whether compound statements ae aligned along with the normal ones. Note this currently applies only to assignments: Align i.e. ``+=`` with ``=``
259

By the way, what is the purpose of FormatStyle::operator==? It looks like it does not compare`BraceWrapping`.

This is an oversight, it should do what bool operator==(const FormatStyle&) const = default; would do, if we had C++ 20.

276

Maybe add another example the other way

a     = 2;
bbb >>= 2;
clang/lib/Format/WhitespaceManager.cpp
469

This shouldn't be necessary if the options are put into AlignConsecutiveStyle.

I'd really want to see simpler tests and everything put into a single AlignConsecutiveAssignment option.

clang/include/clang/Format/Format.h
265

I would even go further (and that I already told the last time). Drop the `Custom` and map the old enums to the struct when parsing, so no new option.

👍

That's my preference too. Having both AlignConsecutiveAssignments and AlignConsecutiveAssignmentsOptions is error-prone.

clang/lib/Format/WhitespaceManager.cpp
469

Yeah, you can get PadAnchors from the Style. Maybe RightJustify too, nope?

clang/unittests/Format/FormatTest.cpp
16806–16816

Do you really need to do such big test cases?
I'd rather see small ones like:

test that <= is not treated as a compound assignment:

aa &= 5;
b <= 10;
c = 15;

etc.

curdeius added inline comments.Feb 14 2022, 2:19 AM
clang/unittests/Format/FormatTest.cpp
16784–16794

No need for long tests. Please simplify other too.

16817

I'd like to see a test that verifies what you put in the documentation:

a   >>= 2;
bbb   = 2;

but also the other way round:

aaa >>= 2;
b     = 2;

and a mix of 1-, 2- and 3-char operators as well.

16909–16911

It should be a separate case.

@sstwcw, would it help if I created a parent revision that only splits the current enum-based option into a struct, so that you only add compound ops and padding?

@curdeius That would be great.

clang/include/clang/Format/Format.h
265

About AlignConsecutiveAssignments and AlignConsecutiveAssignmentsOptions. Based on the current YAML infrastructure, it does not seem possible to support both enum and struct under one name.

curdeius added inline comments.Feb 17 2022, 2:34 AM
clang/include/clang/Format/Format.h
265

Grrr, indeed, that doesn't seem easy. I'm gonna play a bit more with yaml::PolymorphicTraits but not sure it's of any help here.
So yeah, please go on with this revision as if I weren't doing anything :).

sstwcw updated this revision to Diff 410271.Feb 21 2022, 4:17 AM
sstwcw retitled this revision from Add option to align compound assignments like `+=` to [clang-format] Add option to align compound assignments like `+=`.

Use struct for the alignment options.

Herald added a project: Restricted Project. · View Herald TranscriptFeb 21 2022, 4:17 AM
sstwcw marked 3 inline comments as done.Feb 21 2022, 3:09 PM

About the unit tests that failed in B150668. It looks like they were stopped because they took over 1 minute. I ran the first test on my laptop. Both this revision and main took about 2 minutes. What should I do about it?

clang/include/clang/Format/Format.h
259

In the new version all the alignment options can be read as objects. The names are still `AlignConsecutiveMacros` etc.

265

In the new version I added support for it.

sstwcw updated this revision to Diff 410400.Feb 21 2022, 3:11 PM
sstwcw marked 2 inline comments as done.

Nice job! Please let us a bit of time to review that.
Also, I think it would be good to get a reviewer that knows well the yaml parts. Or even split it to a separate revision.
And this part needs tests too.

Could you mark the comments as done (if they are done)

Does the YAML changes need unit tests in (llvm/unittests/Support) ? also we should pull in a reviewer from llvm/Support (maybe @njames93, @grimar ) who have made YAML specific changes in this header.

MyDeveloperDay added inline comments.Feb 22 2022, 6:02 AM
clang/docs/tools/dump_format_style.py
121 ↗(On Diff #410400)

Can this change be separate? why is this needed? Could you add a screengrab of the html that it generates?

clang/lib/Format/WhitespaceManager.cpp
273

something odd here why not, the following its fits 80 columns

// Column - The token for which Matches returns true is moved to this column.
// RightJustify - Whether it is the token's right end or left end that gets
// moved to that column.
336

RightJustify is a bool? so I'm sure this is a good way of doing a conditional but surely there is a compiler out there that is going to complain

curdeius added inline comments.Feb 22 2022, 6:28 AM
clang/docs/ClangFormatStyleOptions.rst
410

Stuff?

532–533
534

Ditto.

560–562

IMO, it would be easier to understand if you appended (after a blank line) the "regardless" example into both true and false part.

1415–1422

That's an unrelated change. Could you please do it in another (NFC) revision?

4203–4211

Ditto.

llvm/docs/YamlIO.rst
561–590 ↗(On Diff #410400)

A simpler example (not clang-format specific) would be better.

llvm/include/llvm/Support/YAMLTraits.h
66 ↗(On Diff #410400)

Can't it be enumeration to match ScalarEnumerationTraits? Or would it clash?

Nice job! Please let us a bit of time to review that.
Also, I think it would be good to get a reviewer that knows well the yaml parts. Or even split it to a separate revision.
And this part needs tests too.

Yeah really thanks for doing this. But please split it up.

clang/lib/Format/Format.cpp
1182

Without the .

sstwcw marked 18 inline comments as done.Feb 22 2022, 3:28 PM

The YAML stuff are now in D120363.

clang/docs/ClangFormatStyleOptions.rst
1415–1422

It's in D120361.

clang/docs/tools/dump_format_style.py
121 ↗(On Diff #410400)

It's in D120361.

sstwcw updated this revision to Diff 413149.Mar 4 2022, 2:52 PM
sstwcw marked 2 inline comments as done.

Take out things in other revisions

Herald added a project: Restricted Project. · View Herald TranscriptMar 4 2022, 2:52 PM
sstwcw updated this revision to Diff 413152.Mar 4 2022, 3:05 PM
sstwcw marked an inline comment as done.
sstwcw marked 7 inline comments as done.Mar 4 2022, 3:10 PM
sstwcw added inline comments.
clang/docs/tools/dump_format_style.py
293 ↗(On Diff #413152)

This change is needed to parse the entry bool PadOperators = true;.

clang/lib/Format/WhitespaceManager.cpp
273

My text editor automatically limited comments to 72 columns.

sstwcw marked an inline comment as done.Mar 4 2022, 3:10 PM
clang/docs/tools/dump_format_style.py
293 ↗(On Diff #413152)

That is a sign that you should keep consistency and initialize PadOperators like the rest.

sstwcw updated this revision to Diff 413251.Mar 5 2022, 1:56 PM
sstwcw marked an inline comment as done.

Only some small things, I think we are nearly done and this is a great change.

clang/include/clang/Format/Format.h
143–144

This fits in one line.

(Please also recheck the other comments.)

247

Please add AlignCompound and PadOperators.

clang/lib/Format/Format.cpp
195
clang/lib/Format/WhitespaceManager.cpp
466

There is no PadAnchors anymore.

clang/unittests/Format/FormatTest.cpp
16890

Can you test it with AlignConsecutiveDeclarations?

19874

Drop the .
Same below.

sstwcw updated this revision to Diff 413271.Mar 5 2022, 9:11 PM
sstwcw marked 5 inline comments as done.Mar 5 2022, 9:19 PM
sstwcw added inline comments.
clang/unittests/Format/FormatTest.cpp
16890

Do you mean like the formatting the same code with AlignConsecutiveAssignments.Enabled being false and AlignConsecutiveDeclarations.Enabled being true? By the way, I just realized that things like int a += 5; are not valid code. Should I remove the int instead?

How does this handle cases with multiple assignments in one statement, and can tests be added to demonstrate the behaviour.

Foo += Bar <<= 5;
Int Baz = 5;
sstwcw added a comment.Mar 6 2022, 3:01 AM

About chained assignments, the current program does not attempt to align them in a consistent way. And this revision doesn't change it. Both before and after this revision, the output depend on the order of the statements.

Foo = Bar = 5;
Int Baz   = 5;

Int Baz = 5;
Foo = Bar = 5;

Is the title making you think it is about chained assignments? I took the term "compound assignment" from Section 6.5.16.2 of C17.

clang/unittests/Format/FormatTest.cpp
16890

No I meant both enabled, with the AlignCompound one time with and without PadOperators. To see if the options work nicely together.

But yeah, as you stated this isn't valid code, so it doen't really matter how it is formatted.

sstwcw updated this revision to Diff 413313.Mar 6 2022, 2:36 PM

remove int in tests

sstwcw marked 2 inline comments as done.Mar 6 2022, 2:38 PM
sstwcw added a comment.Mar 9 2022, 2:33 PM

Any more problems with this revision?

Thanks for your patience. :)

curdeius accepted this revision.Mar 10 2022, 12:36 AM

LGTM. Thanks a lot!

clang/lib/Format/Format.cpp
1181–1190

That should be more future-proof.

sstwcw updated this revision to Diff 414347.Mar 10 2022, 4:53 AM

Modify initialization.

sstwcw marked an inline comment as done.Mar 10 2022, 5:01 AM

@MyDeveloperDay Will you please accept this revision?

MyDeveloperDay accepted this revision.Mar 12 2022, 1:20 AM
This revision is now accepted and ready to land.Mar 12 2022, 1:20 AM

How to get this revision committed? I thought it would happen automatically now that it is accepted.

You either get commit access https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access or ask someone of us to push it for you. In the latter case we need a name and mail for the commit. I recommend the former, especially if you want to contribute more, it's not that hard, just ask. :)

Then you can push to the main branch on Github.

This revision was landed with ongoing or failed builds.Mar 13 2022, 9:47 PM
This revision was automatically updated to reflect the committed changes.