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

[Björn Schäpers via Phabricator via cfe-commits]

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