This is an archive of the discontinued LLVM Phabricator instance.

Differential D35296

[clang-format] Keep level of comment before an empty line
ClosedPublic

Authored by krasimir on Jul 12 2017, 2:36 AM.

Details

Reviewers
djasper
klimek
Commits
rGe092634f31af: [clang-format] Keep level of comment before an empty line
rC307795: [clang-format] Keep level of comment before an empty line
rL307795: [clang-format] Keep level of comment before an empty line
Summary

This patch fixes bug https://bugs.llvm.org/show_bug.cgi?id=3313: a comment line
was aligned with the next #ifdef even in the presence of an empty line between
them.

Diff Detail

Repository
rL LLVM

Event Timeline

krasimir created this revision.Jul 12 2017, 2:36 AM
Herald added a subscriber: klimek. · View Herald TranscriptJul 12 2017, 2:36 AM
djasper edited edge metadata.Jul 12 2017, 3:41 AM

I still think we should make this dependent on the current indentation. Why do you think this is better?

djasper added a reviewer: klimek.Jul 12 2017, 3:41 AM
krasimir updated this revision to Diff 106202.Jul 12 2017, 6:59 AM
  • Switch to original column-based detection
krasimir updated this revision to Diff 106205.Jul 12 2017, 7:06 AM
  • Update switch tests
djasper accepted this revision.Jul 12 2017, 8:09 AM

Some small comments, otherwise looks good.

lib/Format/TokenAnnotator.cpp
1706 ↗(On Diff #106205)

Add a comment a long the lines of:

// If the comment is currently aligned with the line immediately following it,
// that's probably intentional and we should keep it.

And maybe rename s/UpdateLevel/AlignedWithNextLine/

unittests/Format/FormatTest.cpp
855 ↗(On Diff #106205)

Please format this with clang-format.

This revision is now accepted and ready to land.Jul 12 2017, 8:09 AM
krasimir updated this revision to Diff 106220.Jul 12 2017, 8:17 AM
krasimir marked 2 inline comments as done.
  • Address review comments
Closed by commit rL307795: [clang-format] Keep level of comment before an empty line (authored by krasimir). · Explain WhyJul 12 2017, 8:22 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
cfe/
trunk/
lib/
Format/
17 lines
unittests/
Format/
25 lines
64 lines

Diff 106222

cfe/trunk/lib/Format/TokenAnnotator.cpp

Show First 20 Lines • Show All 1,688 Lines • ▼ Show 20 Lines
} // end anonymous namespace } // end anonymous namespace
void TokenAnnotator::setCommentLineLevels( void TokenAnnotator::setCommentLineLevels(
SmallVectorImpl<AnnotatedLine *> &Lines) { SmallVectorImpl<AnnotatedLine *> &Lines) {
const AnnotatedLine *NextNonCommentLine = nullptr; const AnnotatedLine *NextNonCommentLine = nullptr;
for (SmallVectorImpl<AnnotatedLine *>::reverse_iterator I = Lines.rbegin(), for (SmallVectorImpl<AnnotatedLine *>::reverse_iterator I = Lines.rbegin(),
E = Lines.rend(); E = Lines.rend();
I != E; ++I) { I != E; ++I) {
bool CommentLine = (*I)->First; bool CommentLine = true;
for (const FormatToken *Tok = (*I)->First; Tok; Tok = Tok->Next) { for (const FormatToken *Tok = (*I)->First; Tok; Tok = Tok->Next) {
if (!Tok->is(tok::comment)) { if (!Tok->is(tok::comment)) {
CommentLine = false; CommentLine = false;
break; break;
} }
} }
if (NextNonCommentLine && CommentLine)
if (NextNonCommentLine && CommentLine) {
// If the comment is currently aligned with the line immediately following
// it, that's probably intentional and we should keep it.
bool AlignedWithNextLine =
NextNonCommentLine->First->NewlinesBefore <= 1 &&
NextNonCommentLine->First->OriginalColumn ==
(*I)->First->OriginalColumn;
if (AlignedWithNextLine)
(*I)->Level = NextNonCommentLine->Level; (*I)->Level = NextNonCommentLine->Level;
else } else {
NextNonCommentLine = (*I)->First->isNot(tok::r_brace) ? (*I) : nullptr; NextNonCommentLine = (*I)->First->isNot(tok::r_brace) ? (*I) : nullptr;
}
setCommentLineLevels((*I)->Children); setCommentLineLevels((*I)->Children);
} }
} }
static unsigned maxNestingDepth(const AnnotatedLine &Line) { static unsigned maxNestingDepth(const AnnotatedLine &Line) {
unsigned Result = 0; unsigned Result = 0;
for (const auto* Tok = Line.First; Tok != nullptr; Tok = Tok->Next) for (const auto* Tok = Line.First; Tok != nullptr; Tok = Tok->Next)
▲ Show 20 Lines • Show All 1,122 Lines • Show Last 20 Lines

cfe/trunk/unittests/Format/FormatTest.cpp

  • This file is larger than 256 KB, so syntax highlighting is disabled by default.
Show First 20 Lines • Show All 819 Lines • ▼ Show 20 Lines verifyGoogleFormat("Operation codeToOperation(OperationCode OpCode) {\n"
" }\n" " }\n"
"#undef OPERATION_CASE\n" "#undef OPERATION_CASE\n"
"}"); "}");
verifyFormat("DEBUG({\n" verifyFormat("DEBUG({\n"
" switch (x) {\n" " switch (x) {\n"
" case A:\n" " case A:\n"
" f();\n" " f();\n"
" break;\n" " break;\n"
" // On B:\n" " // fallthrough\n"
" case B:\n" " case B:\n"
" g();\n" " g();\n"
" break;\n" " break;\n"
" }\n" " }\n"
"});"); "});");
EXPECT_EQ("DEBUG({\n"
" switch (x) {\n"
" case A:\n"
" f();\n"
" break;\n"
" // On B:\n"
" case B:\n"
" g();\n"
" break;\n"
" }\n"
"});",
format("DEBUG({\n"
" switch (x) {\n"
" case A:\n"
" f();\n"
" break;\n"
" // On B:\n"
" case B:\n"
" g();\n"
" break;\n"
" }\n"
"});",
getLLVMStyle()));
verifyFormat("switch (a) {\n" verifyFormat("switch (a) {\n"
"case (b):\n" "case (b):\n"
" return;\n" " return;\n"
"}"); "}");
verifyFormat("switch (a) {\n" verifyFormat("switch (a) {\n"
"case some_namespace::\n" "case some_namespace::\n"
" some_constant:\n" " some_constant:\n"
▲ Show 20 Lines • Show All 10,249 Lines • Show Last 20 Lines

cfe/trunk/unittests/Format/FormatTestComments.cpp

Show First 20 Lines • Show All 799 Lines • ▼ Show 20 LinesTEST_F(FormatTestComments, ParsesCommentsAdjacentToPPDirectives) {
EXPECT_EQ("namespace {}\n// Test\n#define A", EXPECT_EQ("namespace {}\n// Test\n#define A",
format("namespace {}\n // Test\n#define A")); format("namespace {}\n // Test\n#define A"));
EXPECT_EQ("namespace {}\n/* Test */\n#define A", EXPECT_EQ("namespace {}\n/* Test */\n#define A",
format("namespace {}\n /* Test */\n#define A")); format("namespace {}\n /* Test */\n#define A"));
EXPECT_EQ("namespace {}\n/* Test */ #define A", EXPECT_EQ("namespace {}\n/* Test */ #define A",
format("namespace {}\n /* Test */ #define A")); format("namespace {}\n /* Test */ #define A"));
} }
TEST_F(FormatTestComments, KeepsLevelOfCommentBeforePPDirective) {
// Keep the current level if the comment was originally not aligned with
// the preprocessor directive.
EXPECT_EQ("void f() {\n"
" int i;\n"
" /* comment */\n"
"#ifdef A\n"
" int j;\n"
"}",
format("void f() {\n"
" int i;\n"
" /* comment */\n"
"#ifdef A\n"
" int j;\n"
"}"));
EXPECT_EQ("void f() {\n"
" int i;\n"
" /* comment */\n"
"\n"
"#ifdef A\n"
" int j;\n"
"}",
format("void f() {\n"
" int i;\n"
" /* comment */\n"
"\n"
"#ifdef A\n"
" int j;\n"
"}"));
// Keep the current level if there is an empty line between the comment and
// the preprocessor directive.
EXPECT_EQ("void f() {\n"
" int i;\n"
" /* comment */\n"
"\n"
"#ifdef A\n"
" int j;\n"
"}",
format("void f() {\n"
" int i;\n"
"/* comment */\n"
"\n"
"#ifdef A\n"
" int j;\n"
"}"));
// Align with the preprocessor directive if the comment was originally aligned
// with the preprocessor directive.
EXPECT_EQ("void f() {\n"
" int i;\n"
"/* comment */\n"
"#ifdef A\n"
" int j;\n"
"}",
format("void f() {\n"
" int i;\n"
"/* comment */\n"
"#ifdef A\n"
" int j;\n"
"}"));
}
TEST_F(FormatTestComments, SplitsLongLinesInComments) { TEST_F(FormatTestComments, SplitsLongLinesInComments) {
EXPECT_EQ("/* This is a long\n" EXPECT_EQ("/* This is a long\n"
" * comment that\n" " * comment that\n"
" * doesn't\n" " * doesn't\n"
" * fit on one line.\n" " * fit on one line.\n"
" */", " */",
format("/* " format("/* "
"This is a long " "This is a long "
▲ Show 20 Lines • Show All 1,697 Lines • Show Last 20 Lines