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