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

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added inline comments.


================
Comment at: clang/include/clang/Format/Format.h:157
+  ///   a   &= 2;
+  ///   bbb  = 2;
+  ///
----------------
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.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119599