curdeius added a comment.
I'd really want to see simpler tests and everything put into a single
`AlignConsecutiveAssignment` option.
================
Comment at: clang/include/clang/Format/Format.h:163
+ /// \endcode
+ bool AlignCompoundAssignments;
+
----------------
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.
================
Comment at: clang/lib/Format/WhitespaceManager.cpp:475
+ const FormatStyle::AlignConsecutiveStyle &ACS = FormatStyle::ACS_None,
+ bool RightJustify = false, bool PadAnchors = false) {
+ // We arrange each line in 3 parts. The operator to be aligned (the
----------------
HazardyKnusperkeks wrote:
> This shouldn't be necessary if the options are put into
> `AlignConsecutiveStyle`.
Yeah, you can get `PadAnchors` from the `Style`. Maybe `RightJustify` too, nope?
================
Comment at: clang/unittests/Format/FormatTest.cpp:16400-16410
+ verifyFormat("aa <= 5;\n"
+ "a = 5;\n"
+ "bcd = 5;\n"
+ "ghtyf = 5;\n"
+ "dvfvdb = 5;\n"
+ "a = 5;\n"
+ "vdsvsv = 5;\n"
----------------
No need for long tests. Please simplify other too.
================
Comment at: clang/unittests/Format/FormatTest.cpp:16422-16432
+ verifyFormat("a &= 5;\n"
+ "bcd *= 5;\n"
+ "ghtyf >>= 5;\n"
+ "dvfvdb -= 5;\n"
+ "a /= 5;\n"
+ "aa <= 5;\n"
+ "vdsvsv %= 5;\n"
----------------
Do you really need to do such big test cases?
I'd rather see small ones like:
test that `<=` is not treated as a compound assignment:
```
aa &= 5;
b <= 10;
c = 15;
```
etc.
================
Comment at: clang/unittests/Format/FormatTest.cpp:16433
+ Alignment);
+ Alignment.AlignConsecutiveAssignmentsOptions.PadOperators = true;
+ verifyFormat("aa <= 5;\n"
----------------
I'd like to see a test that verifies what you put in the documentation:
```
a >>= 2;
bbb = 2;
```
but also the other way round:
```
aaa >>= 2;
b = 2;
```
and a mix of 1-, 2- and 3-char operators as well.
================
Comment at: clang/unittests/Format/FormatTest.cpp:16564-16565
Alignment);
- verifyFormat("a &= 5;\n"
+ verifyFormat("aa <= 5;\n"
+ "a &= 5;\n"
"bcd *= 5;\n"
----------------
It should be a separate case.
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
