This is an archive of the discontinued LLVM Phabricator instance.

Differential D35485

[clang-format] Fix comment levels between '}' and PPDirective
ClosedPublic

Authored by krasimir on Jul 17 2017, 7:53 AM.

Details

Reviewers
djasper
Commits
rG06451fa1a22d: [clang-format] Fix comment levels between '}' and PPDirective
rC308725: [clang-format] Fix comment levels between '}' and PPDirective
rL308725: [clang-format] Fix comment levels between '}' and PPDirective
Summary

This fixes a regression exposed by r307795 in which the level of a comment line
between '}' and a preprocessor directive is incorrectly set as the level of the
line before the '}'. In effect, this:

int f(int i) {
  int j = i;
  return i + j;
}
// comment

#ifdef A
#endif

was formatted as:

int f(int i) {
  int j = i;
  return i + j;
}
  // comment

#ifdef A
#endif

Diff Detail

Repository
rL LLVM

Event Timeline

krasimir created this revision.Jul 17 2017, 7:53 AM
Herald added a subscriber: klimek. · View Herald TranscriptJul 17 2017, 7:53 AM
krasimir added a reviewer: djasper.Jul 17 2017, 7:54 AM
krasimir added a subscriber: cfe-commits.
djasper added inline comments.Jul 18 2017, 2:22 AM
lib/Format/UnwrappedLineParser.cpp
489 ↗(On Diff #106871)

What happens if you instead change the Line->Level = InitialLevel; statement from below to before this line? That seems like the more intuitively correct fix.

2378 ↗(On Diff #106871)

LLVM style requires a line break.

unittests/Format/FormatTestComments.cpp
848 ↗(On Diff #106871)

Generally, mess up the code in some way to ensure that it is actually being formatted.

krasimir updated this revision to Diff 107038.Jul 18 2017, 2:58 AM
krasimir marked 2 inline comments as done.
  • Fix formatting
Harbormaster completed remote builds in B8335: Diff 107038.Jul 18 2017, 2:58 AM
krasimir added inline comments.Jul 18 2017, 2:58 AM
lib/Format/UnwrappedLineParser.cpp
489 ↗(On Diff #106871)

This doesn't work since comments before the right brace haven't been emitted yet and would get the wrong level.

unittests/Format/FormatTestComments.cpp
848 ↗(On Diff #106871)

Messing up doesn't work in this case, because we rely on the original columns of the comment and the previous line. That's why I added a bunch of tests.

krasimir updated this revision to Diff 107087.Jul 18 2017, 6:14 AM
  • Remove TODO test case
djasper added inline comments.Jul 18 2017, 7:38 AM
lib/Format/UnwrappedLineParser.cpp
2378 ↗(On Diff #107038)

What happens if we always set the Level to 0 here?

489 ↗(On Diff #106871)

So that means this seems to be the interesting case:

void f() {
  DoSomething();
  // This was a fun function.
}
// Cool macro:
#define A a

Now, both comments are basically read when we are reading the "}", but they should have different indentation levels. I have another suggestion, see below.

unittests/Format/FormatTestComments.cpp
848 ↗(On Diff #106871)

I meant *manually* mess up. So add spaces here and there.

krasimir marked 2 inline comments as done.Jul 18 2017, 8:02 AM
krasimir added inline comments.
lib/Format/UnwrappedLineParser.cpp
2378 ↗(On Diff #107038)

If we always set the Level to 0 here, then

void  f() {
  int i;
  /* comment */

#define A
}

gets indented as

void  f() {
  int i;
/* comment */

#define A
}
489 ↗(On Diff #106871)

Here is another breaking test in case we change Line->Level = InitialLevel to above this line:

switch (x) {
default: {
  // Do nothing.
}
}

gets reformatted as:

switch (x) {
default: {
// Do nothing.
}
}
krasimir updated this revision to Diff 107285.Jul 19 2017, 5:26 AM
krasimir marked 2 inline comments as done.
  • Manually messed up tests
Harbormaster completed remote builds in B8391: Diff 107285.Jul 19 2017, 5:28 AM
djasper added inline comments.Jul 19 2017, 7:43 AM
lib/Format/UnwrappedLineParser.cpp
489 ↗(On Diff #106871)

I think we can fix all of these cases by doing:

flushComments(isOnNewLine(*FormatTok));
Line->Level = InitialLevel;
nextToken(); // Munch the closing brace.

(so add the first two lines here, remove Line->Level = InitialLevel; below.

krasimir updated this revision to Diff 107314.Jul 19 2017, 8:51 AM
  • Adapt the fix from comment suggestion
lib/Format/UnwrappedLineParser.cpp
489 ↗(On Diff #106871)

Cool! Thanks!

Harbormaster completed remote builds in B8397: Diff 107314.Jul 19 2017, 8:53 AM
krasimir added a comment.Jul 21 2017, 1:11 AM

@djasper: ping

djasper accepted this revision.Jul 21 2017, 2:04 AM

Looks good.

This revision is now accepted and ready to land.Jul 21 2017, 2:04 AM
Closed by commit rL308725: [clang-format] Fix comment levels between '}' and PPDirective (authored by krasimir). · Explain WhyJul 21 2017, 3:26 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
cfe/
trunk/
lib/
Format/
3 lines
unittests/
Format/
78 lines

Diff 107648

cfe/trunk/lib/Format/UnwrappedLineParser.cpp

Show First 20 Lines • Show All 480 Lines • ▼ Show 20 Linesvoid UnwrappedLineParser::parseBlock(bool MustBeDeclaration, bool AddLevel,
if (MacroBlock ? !FormatTok->is(TT_MacroBlockEnd) if (MacroBlock ? !FormatTok->is(TT_MacroBlockEnd)
: !FormatTok->is(tok::r_brace)) { : !FormatTok->is(tok::r_brace)) {
Line->Level = InitialLevel; Line->Level = InitialLevel;
FormatTok->BlockKind = BK_Block; FormatTok->BlockKind = BK_Block;
return; return;
} }
flushComments(isOnNewLine(*FormatTok));
Line->Level = InitialLevel;
nextToken(); // Munch the closing brace. nextToken(); // Munch the closing brace.
if (MacroBlock && FormatTok->is(tok::l_paren)) if (MacroBlock && FormatTok->is(tok::l_paren))
parseParens(); parseParens();
if (MunchSemi && FormatTok->Tok.is(tok::semi)) if (MunchSemi && FormatTok->Tok.is(tok::semi))
nextToken(); nextToken();
Line->Level = InitialLevel;
Line->MatchingOpeningBlockLineIndex = OpeningLineIndex; Line->MatchingOpeningBlockLineIndex = OpeningLineIndex;
if (OpeningLineIndex != UnwrappedLine::kInvalidIndex) { if (OpeningLineIndex != UnwrappedLine::kInvalidIndex) {
// Update the opening line to add the forward reference as well // Update the opening line to add the forward reference as well
(*CurrentLines)[OpeningLineIndex].MatchingOpeningBlockLineIndex = (*CurrentLines)[OpeningLineIndex].MatchingOpeningBlockLineIndex =
CurrentLines->size() - 1; CurrentLines->size() - 1;
} }
} }
▲ Show 20 Lines • Show All 1,921 Lines • Show Last 20 Lines

cfe/trunk/unittests/Format/FormatTestComments.cpp

Show First 20 Lines • Show All 830 Lines • ▼ Show 20 Lines EXPECT_EQ("void f() {\n"
format("void f() {\n" format("void f() {\n"
" int i;\n" " int i;\n"
" /* comment */\n" " /* comment */\n"
"\n" "\n"
"#ifdef A\n" "#ifdef A\n"
" int j;\n" " int j;\n"
"}")); "}"));
EXPECT_EQ("int f(int i) {\n"
" if (true) {\n"
" ++i;\n"
" }\n"
" // comment\n"
"#ifdef A\n"
" int j;\n"
"#endif\n"
"}",
format("int f(int i) {\n"
" if (true) {\n"
" ++i;\n"
" }\n"
" // comment\n"
"#ifdef A\n"
"int j;\n"
"#endif\n"
"}"));
// Keep the current level if there is an empty line between the comment and // Keep the current level if there is an empty line between the comment and
// the preprocessor directive. // the preprocessor directive.
EXPECT_EQ("void f() {\n" EXPECT_EQ("void f() {\n"
" int i;\n" " int i;\n"
" /* comment */\n" " /* comment */\n"
"\n" "\n"
"#ifdef A\n" "#ifdef A\n"
" int j;\n" " int j;\n"
"}", "}",
format("void f() {\n" format("void f() {\n"
" int i;\n" " int i;\n"
"/* comment */\n" "/* comment */\n"
"\n" "\n"
"#ifdef A\n" "#ifdef A\n"
" int j;\n" " int j;\n"
"}")); "}"));
EXPECT_EQ("void f() {\n"
" int i;\n"
" return i;\n"
"}\n"
"// comment\n"
"\n"
"#ifdef A\n"
"int i;\n"
"#endif // A",
format("void f() {\n"
" int i;\n"
" return i;\n"
"}\n"
"// comment\n"
"\n"
"#ifdef A\n"
"int i;\n"
"#endif // A"));
EXPECT_EQ("int f(int i) {\n"
" if (true) {\n"
" ++i;\n"
" }\n"
" // comment\n"
"\n"
"#ifdef A\n"
" int j;\n"
"#endif\n"
"}",
format("int f(int i) {\n"
" if (true) {\n"
" ++i;\n"
" }\n"
" // comment\n"
"\n"
"#ifdef A\n"
" int j;\n"
"#endif\n"
"}"));
// Align with the preprocessor directive if the comment was originally aligned // Align with the preprocessor directive if the comment was originally aligned
// with the preprocessor directive. // with the preprocessor directive.
EXPECT_EQ("void f() {\n" EXPECT_EQ("void f() {\n"
" int i;\n" " int i;\n"
"/* comment */\n" "/* comment */\n"
"#ifdef A\n" "#ifdef A\n"
" int j;\n" " int j;\n"
"}", "}",
format("void f() {\n" format("void f() {\n"
" int i;\n" " int i;\n"
"/* comment */\n" "/* comment */\n"
"#ifdef A\n" "#ifdef A\n"
" int j;\n" " int j;\n"
"}")); "}"));
EXPECT_EQ("int f(int i) {\n"
" if (true) {\n"
" ++i;\n"
" }\n"
"// comment\n"
"#ifdef A\n"
" int j;\n"
"#endif\n"
"}",
format("int f(int i) {\n"
" if (true) {\n"
" ++i;\n"
" }\n"
"// comment\n"
"#ifdef A\n"
" int j;\n"
"#endif\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"
" */", " */",
▲ Show 20 Lines • Show All 1,699 Lines • Show Last 20 Lines