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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
