yusuke-kadowaki added inline comments.
================
Comment at: clang/docs/ClangFormatStyleOptions.rst:785
+ - Consecutive
+ - AcrossEmptyLines
+ - AcrossComments
----------------
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.
================
Comment at: clang/docs/ClangFormatStyleOptions.rst:794
+
+ AlignConsecutiveMacros: AcrossEmptyLines
+
----------------
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.
================
Comment at: clang/docs/ClangFormatStyleOptions.rst:797
+ AlignConsecutiveMacros:
+ Enabled: true
+ AcrossEmptyLines: true
----------------
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.
================
Comment at: clang/lib/Format/Format.cpp:649
IO.mapOptional("AlignOperands", Style.AlignOperands);
- IO.mapOptional("AlignTrailingComments", Style.AlignTrailingComments);
IO.mapOptional("AllowAllArgumentsOnNextLine",
----------------
HazardyKnusperkeks wrote:
> MyDeveloperDay wrote:
> > HazardyKnusperkeks wrote:
> > > MyDeveloperDay wrote:
> > > > you can't remove an option, otherwise you'll break everyones
> > > > .clang-format
> > > That's not correct. We have done it:
> > > D108752 -> D108882 -> D127390
> > >
> > > You can remove (and in this case should), but you still need to parse it
> > > and act accordingly. Which is done as far as I can see.
> > sorry thats what I meant, you can remove it, but you have to make it turn
> > on the correct new style that keeps exactly the old user case, and you
> > can't remove it from the configuration parsing otherwise anyone who has it
> > in their .clang-format is immediately broken with an unknown option.
> >
> > to be honest this is an annoyance for introducing new features, which at
> > some point I'd like to drop, you can't have a new option which is not read
> >
> > For me, when we introduced new languages, or new features like InsertBraces
> > etc.. I want to put it in my .clang-format even though other people they
> > aren't using a version that uses it. (becuase it won't impact them), i.e.
> > it won't add braces or correct QualifierOrder that doesn't bother me
> >
> Do you disagree that it actually is parsed?
>
> But that compatibility parsing has nothing to do with ignoring unknown (new)
> options.
I confirmed the old style AlignTrailingComments works and it's also tested with
CHECK_PARSE if I understand correctly.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D132131/new/
https://reviews.llvm.org/D132131
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
