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

[sstwcw via Phabricator via cfe-commits]

sstwcw marked 3 inline comments as done.
sstwcw added a comment.

About the unit tests that failed in B150668 <https://reviews.llvm.org/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?



================
Comment at: clang/include/clang/Format/Format.h:157
+  ///   a   &= 2;
+  ///   bbb  = 2;
+  ///
----------------
HazardyKnusperkeks wrote:
> 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.
> 
> 
In the new version all the alignment options can be read as objects.  The names are still ``AlignConsecutiveMacros`` etc.


================
Comment at: clang/include/clang/Format/Format.h:163
+  /// \endcode
+  bool AlignCompoundAssignments;
+
----------------
curdeius wrote:
> sstwcw wrote:
> > curdeius wrote:
> > > 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.
> > About `AlignConsecutiveAssignments` and `AlignConsecutiveAssignmentsOptions`. Based on the current YAML infrastructure, it does not seem possible to support both enum and struct under one name.
> 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 :).
In the new version I added support for it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119599