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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
