[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:797
+    AlignConsecutiveMacros:
+      Enabled: true
+      AcrossEmptyLines: true
----------------
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..)


================
Comment at: clang/lib/Format/Format.cpp:649
     IO.mapOptional("AlignOperands", Style.AlignOperands);
-    IO.mapOptional("AlignTrailingComments", Style.AlignTrailingComments);
     IO.mapOptional("AllowAllArgumentsOnNextLine",
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132131