HazardyKnusperkeks added inline comments.
================
Comment at: clang/docs/ClangFormatStyleOptions.rst:785
+ - Consecutive
+ - AcrossEmptyLines
+ - AcrossComments
----------------
yusuke-kadowaki wrote:
> MyDeveloperDay wrote:
> > may be AcrossEmptyLines should be a number (to mean the number of empty
> > lines)
> We need to introduce a new struct to do that since AlignConsecutiveStyle is
> shared with some options and not possible to be changed. Plus I think more
> than two empty lines are formatted to one empty line anyway.
There `MaxEmptyLinesToKeep` which would allow to set it higher.
If we want to change the `bool` to an `unsigned`, then that should be a
different change.
================
Comment at: clang/docs/ClangFormatStyleOptions.rst:794
+
+ AlignConsecutiveMacros: AcrossEmptyLines
+
----------------
yusuke-kadowaki wrote:
> MyDeveloperDay wrote:
> > Should this say `AlignedConsecutuveCommets`
> No. This part is a documentation about AlignConsecutiveStyle type, which is
> appended automatically after all the options that take AlignConsecutiveStyle
> type.
Which we could change to reflect that it's used for multiple options.
================
Comment at: clang/docs/ClangFormatStyleOptions.rst:797
+ AlignConsecutiveMacros:
+ Enabled: true
+ AcrossEmptyLines: true
----------------
yusuke-kadowaki wrote:
> MyDeveloperDay wrote:
> > HazardyKnusperkeks wrote:
> > > MyDeveloperDay wrote:
> > > > why do we need Enabled?
> > > >
> > > > isn't it just
> > > >
> > > > ```
> > > > false:
> > > > AcrossEmptyLines: false
> > > > AcrossComments: false
> > > >
> > > > true:
> > > > AcrossEmptyLines: true
> > > > AcrossComments: true
> > > > ```
> > > The struct is a bit older. And we need `Enabled`, one might wish to align
> > > only really consecutive comments, as the option right now does. (Same for
> > > macros, assignments, etc..)
> > I'm still not sure I understand, plus are those use cases you describe
> > tested, I couldn't see that.
> >
> > If feels like what you are saying is that AlignConsecutiveStyle is used
> > elsewhere and it has options that don't pertain to aligning comments?
> >
> > I couldn't tell if feel like this documentation is describing something
> > other than AligningTrailingComments, if I'm confused users could be too?
> As for the Enabled option,
> Enabled: true
> AcrossEmptyLines: false
>
> is supposed to work in the same way as the old style AlignTrailingComments.
> So the tests of AlignTrailingComments are the test cases.
>
> For the documentation, I also thought it was a bit confusing when I first saw
> it because the description of the option and the style is kind of connected.
> But as I also mentioned above, this part is automatically append and it
> affects all the other options that have AlignConsecutiveStyle if we change.
> So I think it should be another patch even if we are to modify it.
> I'm still not sure I understand, plus are those use cases you describe
> tested, I couldn't see that.
>
> If feels like what you are saying is that AlignConsecutiveStyle is used
> elsewhere and it has options that don't pertain to aligning comments?
>
> I couldn't tell if feel like this documentation is describing something other
> than AligningTrailingComments, if I'm confused users could be too?
It was added in D93986 as an enum, and in D119599 made into a struct which then
had 2 options only valid for assignments, not the macros or declarations. Both
accepted by you.
So I see no problem, but think it is the right thing, to port aligning comments
to the struct, even if the flag `AccrossComments` is a noop in this case. When
this lands I actually plan to add a flag only used by comments.
================
Comment at: clang/unittests/Format/FormatTestComments.cpp:2863
+ FormatStyle Style = getLLVMStyle();
+ Style.AlignConsecutiveTrailingComments.AcrossEmptyLines = true;
+ verifyFormat("#include \"a.h\" // simple\n"
----------------
Interesting would be a comment which is split, do we continue to align, or not?
================
Comment at: clang/unittests/Format/FormatTestComments.cpp:2958
+ " long b;",
+ Style));
+}
----------------
Trailing whitespace.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D132131/new/
https://reviews.llvm.org/D132131
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
