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