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

[Yusuke Kadowaki via Phabricator via cfe-commits]

yusuke-kadowaki marked 7 inline comments as done.
yusuke-kadowaki added inline comments.


================
Comment at: clang/docs/ClangFormatStyleOptions.rst:865
+
+  * ``TCAS_DontAlign`` (in configuration: ``DontAlign``)
+    Don't align trailing comments.
----------------
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) 




================
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:
> 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?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132131