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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to