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

Thu Sep 8 12:35:11 PDT 2022

HazardyKnusperkeks added a comment.

In D132131#3776956 <https://reviews.llvm.org/D132131#3776956>, @MyDeveloperDay wrote:

> an example of the exact reason why we should not reuse the same struct...   https://github.com/llvm/llvm-project/issues/57464

Just a bit rephrasing.

In D132131#3777004 <https://reviews.llvm.org/D132131#3777004>, @MyDeveloperDay wrote:

> another reason for using a struct, is that we might want to say something like "AlignNamespaceTrainingComments:false" https://github.com/llvm/llvm-project/issues/57504

Something like that we really want, and that is something I'd like to put on top of this change. Also for all comments following a `}`. That's the only reason I've disabled aligning comments for now.

================
Comment at: clang/docs/ClangFormatStyleOptions.rst:865
+
+  * ``TCAS_DontAlign`` (in configuration: ``DontAlign``)
+    Don't align trailing comments.
----------------
yusuke-kadowaki wrote:
> HazardyKnusperkeks wrote:
> > MyDeveloperDay wrote:
> > > yusuke-kadowaki wrote:
> > > > MyDeveloperDay wrote:
> > > > > Is Don'tAlign the same as Leave that other options support  (for options it best if we try and use the same terminiology
> > > > > 
> > > > > Always,Never,Leave (if that fits)
> > > > IMHO, `Leave` probably fits but `DontAlign`  is more straightforward. Also `DontAlign` is used for other alignment styles like `BracketAlignmentStyle` so it would be more natural.
> > > Leave should mean do nothing.. I'm personally not a fan of DontAlign because obvious there should be a ' between the n and the t but I see we use it elsewhere
> > > 
> > > But actually now I see your DontAlign is effectively the as before (i.e. it will not add extra spaces) 
> > > 
> > > The point of Leave is what people have been asking for when we introduce a new option, that is we if possible add an option which means "Don't touch it at all"  i.e.  if I want to have
> > > 
> > > ```
> > > int a;           //  abc
> > > int b;   /// bcd
> > > int c;                                    // cde
> > > ```
> > > 
> > > Then so be it
> > > 
> > > 
> > Leave is a nice option, yes.
> > I think it is complementary to `Dont`.
> > 
> > But maybe if the option is called `AlignTrailingComments` one may interpret `Leave` not as "don't touch the position of a comment at all" (what do we do, if the comment is outside of the column limit?), but as "just don't touch them, when they are somewhat aligned". Just a thought.
> > Leave should mean do nothing
> 
> Ok now I see what `Leave` means. 
> But that should be another piece of work no? 
> 
> Of course me or someone can add the feature later (I don't really know how to implement that though at least for now) 
> 
> 
Fine by me.

================
Comment at: clang/include/clang/Format/Format.h:373
+  /// Different styles for aligning trailing comments.
+  enum TrailingCommentsAlignmentStyle : int8_t {
+    /// Don't align trailing comments.
----------------
yusuke-kadowaki wrote:
> HazardyKnusperkeks wrote:
> > MyDeveloperDay wrote:
> > > yusuke-kadowaki wrote:
> > > > MyDeveloperDay wrote:
> > > > > all options have a life cycle
> > > > > 
> > > > > bool -> enum -> struct
> > > > > 
> > > > > One of the thing I ask you before, would we want to align across N empty lines, if ever so they I think
> > > > > we should go straight to a struct
> > > > > all options have a life cycle
> > > > 
> > > > I see your point. But we are checking `Style.MaxEmptyLinesToKeep` to determine how many consecutive lines to align. So currently no need to specify it from the option. You'll find the implementation below.
> > > I think its a bad idea to hijack a different option to do this..I don't think it means the same thing and I think what whilst it might work there will be someone somewhere who doesn't want it to behave like this and will call it out as a bug.
> > > 
> > > 
> > Since you are strictly against `Enabled` what to put into a struct?
> > Since you are strictly against Enabled what to put into a struct?
> 
> @MyDeveloperDay 
> Can you answer this? Plus it would be helpful if you just write down what your ideal struct be like.
> 
> > I don't think it means the same thing
> 
> How so? There's no empty lines more than `MaxEmptyLinesToKeep` all the time no?
> 
> > I don't think it means the same thing
> 
> How so? There's no empty lines more than `MaxEmptyLinesToKeep` all the time no?
> 

But you could set `MaxEmptyLinesToKeep` to 3 and aligning comments to over 2 empty lines.
```
int thisIsAVariable = 5; // It Really is

int stillAligned = 2;    // Yep

int notSoMuch = 2; // Nope
```
It certainly doesn't hurt to have a value there.

================
Comment at: clang/include/clang/Format/Format.h:373
+  /// Different styles for aligning trailing comments.
+  enum TrailingCommentsAlignmentStyle : int8_t {
+    /// Don't align trailing comments.
----------------
HazardyKnusperkeks wrote:
> yusuke-kadowaki wrote:
> > HazardyKnusperkeks wrote:
> > > MyDeveloperDay wrote:
> > > > yusuke-kadowaki wrote:
> > > > > MyDeveloperDay wrote:
> > > > > > all options have a life cycle
> > > > > > 
> > > > > > bool -> enum -> struct
> > > > > > 
> > > > > > One of the thing I ask you before, would we want to align across N empty lines, if ever so they I think
> > > > > > we should go straight to a struct
> > > > > > all options have a life cycle
> > > > > 
> > > > > I see your point. But we are checking `Style.MaxEmptyLinesToKeep` to determine how many consecutive lines to align. So currently no need to specify it from the option. You'll find the implementation below.
> > > > I think its a bad idea to hijack a different option to do this..I don't think it means the same thing and I think what whilst it might work there will be someone somewhere who doesn't want it to behave like this and will call it out as a bug.
> > > > 
> > > > 
> > > Since you are strictly against `Enabled` what to put into a struct?
> > > Since you are strictly against Enabled what to put into a struct?
> > 
> > @MyDeveloperDay 
> > Can you answer this? Plus it would be helpful if you just write down what your ideal struct be like.
> > 
> > > I don't think it means the same thing
> > 
> > How so? There's no empty lines more than `MaxEmptyLinesToKeep` all the time no?
> > 
> > > I don't think it means the same thing
> > 
> > How so? There's no empty lines more than `MaxEmptyLinesToKeep` all the time no?
> > 
> 
> But you could set `MaxEmptyLinesToKeep` to 3 and aligning comments to over 2 empty lines.
> ```
> int thisIsAVariable = 5; // It Really is
> 
> 
> int stillAligned = 2;    // Yep
> 
> 
> 
> int notSoMuch = 2; // Nope
> ```
> It certainly doesn't hurt to have a value there.
> > Since you are strictly against Enabled what to put into a struct?
> 
> @MyDeveloperDay 
> Can you answer this? Plus it would be helpful if you just write down what your ideal struct be like.
> 

For me it would currently look like:
```
struct Style {
  enum E {
    Yes,
    No,
  };
  E State;
  unsigned OverEmptyLines;
}
```
Names are open for debate.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132131/new/

https://reviews.llvm.org/D132131