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

[sstwcw via Phabricator via cfe-commits]

sstwcw added a comment.

@curdeius That would be great.



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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119599