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

[Daniel Jasper via Phabricator via cfe-commits]

djasper added inline comments.


================
Comment at: lib/Format/WhitespaceManager.cpp:287
                             SmallVector<WhitespaceManager::Change, 16> &Changes,
+                            bool ConsiderScope, bool ConsiderCommas,
                             unsigned StartAt) {
----------------
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.


================
Comment at: lib/Format/WhitespaceManager.cpp:396
+
+  if (!Keyword || !Current->is(tok::identifier))
+    return false;
----------------
It's probably just preference, but I find the order here confusing. How about:

  if (!Keyword || !Keyword->is(tok::pp_define))
    return false;
  return Current->is(tok::identifier) && Current->Next && Current->Next->SpacesRequiredBefore;

?


================
Comment at: lib/Format/WhitespaceManager.cpp:413
+
+  while (Param && !Param->is(tok::l_paren)) {
+    if (!Param->is(tok::identifier) && !Param->is(tok::comma))
----------------
I think you should be able to use Current.MatchingParen here.


================
Comment at: lib/Format/WhitespaceManager.cpp:427
+
+  Keyword = Param->Previous->Previous;
+  return Keyword && Keyword->is(tok::pp_define);
----------------
How about:

  return endsMacroIdentifier(Param->Previous);

?


================
Comment at: lib/Format/WhitespaceManager.cpp:464
               },
-              Changes, /*StartAt=*/0);
+              Changes, true, true, /*StartAt=*/0);
 }
----------------
As for StartAt, use a comment to name the parameter.


================
Comment at: unittests/Format/FormatTest.cpp:7521
 
+TEST_F(FormatTest, AlignConsecutiveMacros) {
+  FormatStyle Alignment = getLLVMStyle();
----------------
I think it might be useful to create tests for multi-line macros.


================
Comment at: unittests/Format/FormatTest.cpp:7522
+TEST_F(FormatTest, AlignConsecutiveMacros) {
+  FormatStyle Alignment = getLLVMStyle();
+  Alignment.AlignConsecutiveAssignments = true;
----------------
Either call this "Style" instead of "Alignment" or create two styles, Alignment and NoAlignment.


================
Comment at: unittests/Format/FormatTest.cpp:7733
 
-  verifyFormat("auto lambda = []() {\n"
+  verifyFormat("#define a         5\n"
+               "#define foo(x, y) (x + y)\n"
----------------
Why this change? This seems to be tested above.


================
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"
----------------
Same here?


Repository:
  rL LLVM

https://reviews.llvm.org/D28462