[PATCH] D119599: Add option to align compound assignments like `+=`

[sstwcw via Phabricator via cfe-commits]

sstwcw marked 6 inline comments as done.
sstwcw added inline comments.


================
Comment at: clang/include/clang/Format/Format.h:151
 
+  /// When aligning assignments, whether compound assignments like
+  /// ``+=``'s are aligned along with ``=``'s.
----------------
curdeius wrote:
> You need to update the RST files when updating the doc comments here. Please use https://github.com/llvm/llvm-project/blob/main/clang/docs/tools/dump_format_style.py for that.
I guess it is saying I need to say what version the option first appeared in.  How do I know what version it will be?


================
Comment at: clang/include/clang/Format/Format.h:157
+  ///   a   &= 2;
+  ///   bbb  = 2;
+  ///
----------------
HazardyKnusperkeks wrote:
> curdeius wrote:
> > I guess it would be complicated to avoid adding an additional space here. I mean, it could be:
> > ```
> > a  &= 2;
> > bbb = 2;
> > ```
> > And with 3-char operators, there's one more space.
> That would be awesome, but it should be an option to turn off or on.
> But I think this would really be complicated.
I can do it either way. But I thought without the extra space the formatted code looked ugly, especially when mixing `>>=` and `=`.  Which way do you prefer?


```
a >>= 2;
bbb = 2;
```


================
Comment at: clang/lib/Format/WhitespaceManager.cpp:580
 
-    unsigned ChangeMinColumn = Changes[i].StartOfTokenColumn;
-    int LineLengthAfter = Changes[i].TokenLength;
+    unsigned ChangeWidthLeft = Changes[i].StartOfTokenColumn;
+    unsigned ChangeWidthAnchor = 0;
----------------
MyDeveloperDay wrote:
> I like the final outcome, I'm uneasy about renaming all the variables, just because you now understand them. I'm struggling to read the algorithm in the same context as the prior version
> 
> 
Originally we tracked the column to put the operators to, so `ChangeMinColumn` and `ChangeMaxColumn` made sense.  Now the column we are tracking is no longer the column to which we change the start of the operators to, the original names don't make sense any more.


================
Comment at: clang/unittests/Format/FormatTest.cpp:16852
-  // otherwise the function parameters will be re-flowed onto a single line.
-  Alignment.ColumnLimit = 0;
   EXPECT_EQ("int    a(int   x,\n"
----------------
MyDeveloperDay wrote:
> Huge `no` from me, don't change the tests because they break based on your change.
Here is the problem in isolation: https://reviews.llvm.org/D119625.  What do you suggest about this test?


================
Comment at: clang/unittests/Format/FormatTest.cpp:16872
+  // that things still get aligned.
+  Alignment.ColumnLimit = 20;
   EXPECT_EQ("int    a(int   x,\n"
----------------
curdeius wrote:
> Is it something that can be done/fixed separately?
I added a revision here https://reviews.llvm.org/D119625.  If you commit it separately there will be a merge conflict though.


================
Comment at: clang/unittests/Format/FormatTest.cpp:19277
   CHECK_PARSE_BOOL(AllowShortLoopsOnASingleLine);
+  CHECK_PARSE_BOOL(AlignTrailingComments);
   CHECK_PARSE_BOOL(BinPackArguments);
----------------
MyDeveloperDay wrote:
> `IJKL` last L looked i came before l right? so why did you move this down
Sorry.  I forgot my ABC's.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119599