[PATCH] D119599: Add option to align compound assignments like `+=`

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

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



================
Comment at: clang/include/clang/Format/Format.h:163
+  /// \endcode
+  bool AlignCompoundAssignments;
+
----------------
HazardyKnusperkeks wrote:
> MyDeveloperDay wrote:
> > 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?     
> > 
> > 
> > 
> 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.
> 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.

:+1:

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


================
Comment at: clang/lib/Format/WhitespaceManager.cpp:475
+    const FormatStyle::AlignConsecutiveStyle &ACS = FormatStyle::ACS_None,
+    bool RightJustify = false, bool PadAnchors = false) {
+  // We arrange each line in 3 parts. The operator to be aligned (the
----------------
HazardyKnusperkeks wrote:
> This shouldn't be necessary if the options are put into `AlignConsecutiveStyle`.
Yeah, you can get `PadAnchors` from the `Style`. Maybe `RightJustify` too, nope?


================
Comment at: clang/unittests/Format/FormatTest.cpp:16400-16410
+  verifyFormat("aa <= 5;\n"
+               "a               = 5;\n"
+               "bcd             = 5;\n"
+               "ghtyf           = 5;\n"
+               "dvfvdb          = 5;\n"
+               "a               = 5;\n"
+               "vdsvsv          = 5;\n"
----------------
No need for long tests. Please simplify other too.


================
Comment at: clang/unittests/Format/FormatTest.cpp:16422-16432
+  verifyFormat("a      &= 5;\n"
+               "bcd    *= 5;\n"
+               "ghtyf >>= 5;\n"
+               "dvfvdb -= 5;\n"
+               "a      /= 5;\n"
+               "aa <= 5;\n"
+               "vdsvsv         %= 5;\n"
----------------
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.


================
Comment at: clang/unittests/Format/FormatTest.cpp:16433
+               Alignment);
+  Alignment.AlignConsecutiveAssignmentsOptions.PadOperators = true;
+  verifyFormat("aa <= 5;\n"
----------------
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.


================
Comment at: clang/unittests/Format/FormatTest.cpp:16564-16565
                Alignment);
-  verifyFormat("a &= 5;\n"
+  verifyFormat("aa <= 5;\n"
+               "a &= 5;\n"
                "bcd *= 5;\n"
----------------
It should be a separate case.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119599/new/

https://reviews.llvm.org/D119599