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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits