[PATCH] D93986: [clang-format] Add the possibility to align assignments spanning empty lines or comments

[Lukas Barth via Phabricator via cfe-commits]

tinloaf marked 5 inline comments as done.
tinloaf added inline comments.


================
Comment at: clang/include/clang/Format/Format.h:130
+    /// \endcode
+    ACA_AcrossComments
+  };
----------------
MyDeveloperDay wrote:
> tinloaf wrote:
> > MyDeveloperDay wrote:
> > > Is there a case for aligning over empty lines and comments?
> > > 
> > > ```
> > > int a           = 5;
> > > 
> > > /* comment */
> > > int oneTwoThree = 123;
> > > ```
> > Not sure I understand what you mean. Currently, the Option `AcrossComments` includes 'across empty lines'. So, there currently is no case for "across comments, but not across empty lines". I'm not sure if that is really something people want to do. Do you think so? I can add it. 
> I could see a case where you might want to begin a new "alignment group" by leaving a blank line.
> 
> ```
> /* align these 3 */
> int a = 5;
> /* align these 3 */
> int b = 6;
> /* align these 3 */
> int c = 7;
> 
> /* align these 2 which are longer */
> int d           = 5
> /* align these 2 which are longer */
> int oneTwoThree = 123;
> ```
Yes, good point. I've added the option `AlignAcrossEmptyLinesAndComments`, and made `AlignAcrossComments` do what the name suggests.


================
Comment at: clang/lib/Format/WhitespaceManager.cpp:367
+                            unsigned StartAt, bool AcrossEmpty = false,
+                            bool AcrossComments = false) {
   unsigned MinColumn = 0;
----------------
MyDeveloperDay wrote:
> could this be an enum?
> 
> ```
> enum {
>     None
>     AcrossEmptyLines,
>     AcrossComments,
>     AcrossEmptyLinesAndComments,
> }
> ```
Yes, that's probably cleaner, though that means I need a rather ugly `switch` statement to convert one enum into the other where `AlignTokens` is called. Maybe one could address this with some template magic, but that's probably not less ugly than the switch statement.


================
Comment at: clang/lib/Format/WhitespaceManager.cpp:421
       // matching token, the sequence ends here.
-      if (Changes[i].NewlinesBefore > 1 || !FoundMatchOnLine)
+      if (((Changes[i].NewlinesBefore > 1) && (!AcrossEmpty)) ||
+          (!FoundMatchOnLine && (!(LineIsComment && AcrossComments))))
----------------
curdeius wrote:
> Nit: unnecessary parentheses around !AcrossEmpty.
> Same around !(LineIsComment && AcrossComments).
> Maybe you might factor out a bool variable for this condition?
Good point. I factored this out into two booleans, which should improve readability.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93986