[PATCH] D28462: clang-format: Add new style option AlignConsecutiveMacros

[Erik Nyquist via Phabricator via cfe-commits]

enyquist marked 8 inline comments as done.
enyquist added inline comments.


================
Comment at: lib/Format/WhitespaceManager.cpp:287
                             SmallVector<WhitespaceManager::Change, 16> &Changes,
+                            bool ConsiderScope, bool ConsiderCommas,
                             unsigned StartAt) {
----------------
djasper wrote:
> I don't find it intuitive what "consider" means in this case. Can you add a comment? Are the two parameters ever set independently? If not, I'd prefer to keep one parameter for now as we have test coverage only for that.
Currently, they are never set independently, no. I will combine them and add an explanatory comment.


================
Comment at: lib/Format/WhitespaceManager.cpp:413
+
+  while (Param && !Param->is(tok::l_paren)) {
+    if (!Param->is(tok::identifier) && !Param->is(tok::comma))
----------------
djasper wrote:
> I think you should be able to use Current.MatchingParen here.
Hmm, I couldn't make this work... I just replaced this line with

    while (Param && Param != Current->MatchingParen)

But it must not be doing what I think it's doing, since it made some tests fail.
Mind you, my C-brain might be taking over here, please let me know if I'm using MatchingParen incorrectly


================
Comment at: unittests/Format/FormatTest.cpp:7733
 
-  verifyFormat("auto lambda = []() {\n"
+  verifyFormat("#define a         5\n"
+               "#define foo(x, y) (x + y)\n"
----------------
djasper wrote:
> Why this change? This seems to be tested above.
I wanted to ensure this option didn't break other alignment options while I was developing-- this is a remnant. I will remove it.


================
Comment at: unittests/Format/FormatTest.cpp:8010
   Alignment.AlignConsecutiveAssignments = true;
-  verifyFormat("auto lambda = []() {\n"
+  verifyFormat("#define a         5\n"
+               "#define foo(x, y) (x + y)\n"
----------------
djasper wrote:
> Same here?
Yes.


Repository:
  rL LLVM

https://reviews.llvm.org/D28462