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

Mon Feb 14 00:05:00 PST 2022

HazardyKnusperkeks added a comment.

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

================
Comment at: clang/include/clang/Format/Format.h:157
+  ///   a   &= 2;
+  ///   bbb  = 2;
+  ///
----------------
sstwcw wrote:
> HazardyKnusperkeks wrote:
> > sstwcw wrote:
> > > HazardyKnusperkeks wrote:
> > > > sstwcw wrote:
> > > > > HazardyKnusperkeks wrote:
> > > > > > curdeius wrote:
> > > > > > > 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.
> > > > > > That would be awesome, but it should be an option to turn off or on.
> > > > > > But I think this would really be complicated.
> > > > > 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;
> > > > > ```
> > > > 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.
> > > 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.
> > 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.
> > };
> > ```
> 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`.
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 ``=`````

================
Comment at: clang/include/clang/Format/Format.h:157
+  ///   a   &= 2;
+  ///   bbb  = 2;
+  ///
----------------
HazardyKnusperkeks wrote:
> sstwcw wrote:
> > HazardyKnusperkeks wrote:
> > > sstwcw wrote:
> > > > HazardyKnusperkeks wrote:
> > > > > sstwcw wrote:
> > > > > > HazardyKnusperkeks wrote:
> > > > > > > curdeius wrote:
> > > > > > > > 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.
> > > > > > > That would be awesome, but it should be an option to turn off or on.
> > > > > > > But I think this would really be complicated.
> > > > > > 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;
> > > > > > ```
> > > > > 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.
> > > > 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.
> > > 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.
> > > };
> > > ```
> > 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`.
> 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 ``=`````
>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.

================
Comment at: clang/include/clang/Format/Format.h:174
+    ///   false:
+    ///   a >>= 2;
+    ///   bbb = 2;
----------------
Maybe add another example the other way
```
a     = 2;
bbb >>= 2;
```

================
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
----------------
This shouldn't be necessary if the options are put into `AlignConsecutiveStyle`.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119599