[PATCH] D132131: [clang-format] Adds a formatter for aligning trailing comments over empty lines

[Yusuke Kadowaki via Phabricator via cfe-commits]

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