This is an archive of the discontinued LLVM Phabricator instance.

Differential D35794

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

Authored by krasimir on Jul 24 2017, 4:33 AM.

Details

Reviewers
djasper
klimek
Commits
rG3e0510548638: [clang-format] Fix comment levels between '} else {' and PPDirective.
rC308882: [clang-format] Fix comment levels between '} else {' and PPDirective.
rL308882: [clang-format] Fix comment levels between '} else {' and PPDirective.
Summary

This fixes a regression exposed by r307795 and rL308725 in which the level of a
comment line between '} else {' and a preprocessor directive is incorrectly set
as the level of the '} else {' line. For example, this :

int f(int i) {
  if (i) {
    ++i;
  } else {
    // comment
#ifdef A
    --i;
#endif
  }
}

was formatted as:

int f(int i) {
  if (i) {
    ++i;
  } else {
  // comment
#ifdef A
    --i;
#endif
  }
}

Diff Detail

Event Timeline

krasimir created this revision.Jul 24 2017, 4:33 AM
Herald added a subscriber: klimek. · View Herald TranscriptJul 24 2017, 4:33 AM
krasimir updated this revision to Diff 107896.Jul 24 2017, 6:51 AM
  • Make the closing brace use the same thing
krasimir updated this revision to Diff 107900.Jul 24 2017, 7:30 AM
  • Switch to relative levels
krasimir added a reviewer: klimek.Jul 24 2017, 7:30 AM
krasimir updated this revision to Diff 107901.Jul 24 2017, 7:31 AM
  • Remove unused import
klimek accepted this revision.Jul 24 2017, 7:42 AM

lg

lib/Format/UnwrappedLineParser.h
127

s/this token/the current token/
"This token" can be interpreted as the one nextToken reads otherwise.

This revision is now accepted and ready to land.Jul 24 2017, 7:42 AM
krasimir updated this revision to Diff 107904.Jul 24 2017, 7:49 AM
  • Address review comments
Harbormaster completed remote builds in B8530: Diff 107904.Jul 24 2017, 7:49 AM
Closed by commit rL308882: [clang-format] Fix comment levels between '} else {' and PPDirective. (authored by krasimir). · Explain WhyJul 24 2017, 7:54 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
lib/
Format/
2 lines
5 lines
19 lines
unittests/
Format/
157 lines

Diff 107896

lib/Format/TokenAnnotator.cpp

Show First 20 Lines • Show All 2,814 Lines • ▼ Show 20 Lines return Left.isOneOf(tok::comma, tok::coloncolon, tok::semi, tok::l_brace,
tok::colon, tok::l_square, tok::at) || tok::colon, tok::l_square, tok::at) ||
(Left.is(tok::r_paren) && (Left.is(tok::r_paren) &&
Right.isOneOf(tok::identifier, tok::kw_const)) || Right.isOneOf(tok::identifier, tok::kw_const)) ||
(Left.is(tok::l_paren) && !Right.is(tok::r_paren)) || (Left.is(tok::l_paren) && !Right.is(tok::r_paren)) ||
(Left.is(TT_TemplateOpener) && !Right.is(TT_TemplateCloser)); (Left.is(TT_TemplateOpener) && !Right.is(TT_TemplateCloser));
} }
void TokenAnnotator::printDebugInfo(const AnnotatedLine &Line) { void TokenAnnotator::printDebugInfo(const AnnotatedLine &Line) {
llvm::errs() << "AnnotatedTokens:\n"; llvm::errs() << "AnnotatedTokens(L=" << Line.Level << "):\n";
const FormatToken *Tok = Line.First; const FormatToken *Tok = Line.First;
while (Tok) { while (Tok) {
llvm::errs() << " M=" << Tok->MustBreakBefore llvm::errs() << " M=" << Tok->MustBreakBefore
<< " C=" << Tok->CanBreakBefore << " C=" << Tok->CanBreakBefore
<< " T=" << getTokenTypeName(Tok->Type) << " T=" << getTokenTypeName(Tok->Type)
<< " S=" << Tok->SpacesRequiredBefore << " S=" << Tok->SpacesRequiredBefore
<< " B=" << Tok->BlockParameterCount << " B=" << Tok->BlockParameterCount
<< " BK=" << Tok->BlockKind << " BK=" << Tok->BlockKind
Show All 16 Lines

lib/Format/UnwrappedLineParser.h

Show All 14 Lines
#ifndef LLVM_CLANG_LIB_FORMAT_UNWRAPPEDLINEPARSER_H #ifndef LLVM_CLANG_LIB_FORMAT_UNWRAPPEDLINEPARSER_H
#define LLVM_CLANG_LIB_FORMAT_UNWRAPPEDLINEPARSER_H #define LLVM_CLANG_LIB_FORMAT_UNWRAPPEDLINEPARSER_H
#include "FormatToken.h" #include "FormatToken.h"
#include "clang/Basic/IdentifierTable.h" #include "clang/Basic/IdentifierTable.h"
#include "clang/Format/Format.h" #include "clang/Format/Format.h"
#include "llvm/Support/Regex.h" #include "llvm/Support/Regex.h"
#include "llvm/ADT/Optional.h"
#include <list> #include <list>
#include <stack> #include <stack>
namespace clang { namespace clang {
namespace format { namespace format {
struct UnwrappedLineNode; struct UnwrappedLineNode;
▲ Show 20 Lines • Show All 87 Lines • ▼ Show 20 Linesprivate:
void parseObjCInterfaceOrImplementation(); void parseObjCInterfaceOrImplementation();
void parseObjCProtocol(); void parseObjCProtocol();
void parseJavaScriptEs6ImportExport(); void parseJavaScriptEs6ImportExport();
bool tryToParseLambda(); bool tryToParseLambda();
bool tryToParseLambdaIntroducer(); bool tryToParseLambdaIntroducer();
void tryToParseJSFunction(); void tryToParseJSFunction();
void addUnwrappedLine(); void addUnwrappedLine();
bool eof() const; bool eof() const;
void nextToken(); void nextToken(llvm::Optional<unsigned> LevelOfCommentLinesAfter = None);
klimekUnsubmitted
Not Done
ReplyInline Actions

s/this token/the current token/
"This token" can be interpreted as the one nextToken reads otherwise.

klimek: s/this token/the current token/ "This token" can be interpreted as the one nextToken reads…
const FormatToken *getPreviousToken(); const FormatToken *getPreviousToken();
void readToken(); void readToken(llvm::Optional<unsigned> LevelOfCommentLinesAfter = None);
// Decides which comment tokens should be added to the current line and which // Decides which comment tokens should be added to the current line and which
// should be added as comments before the next token. // should be added as comments before the next token.
// //
// Comments specifies the sequence of comment tokens to analyze. They get // Comments specifies the sequence of comment tokens to analyze. They get
// either pushed to the current line or added to the comments before the next // either pushed to the current line or added to the comments before the next
// token. // token.
// //
▲ Show 20 Lines • Show All 116 Lines • Show Last 20 Lines

lib/Format/UnwrappedLineParser.cpp

Show First 20 Lines • Show All 454 Lines • ▼ Show 20 Lines
void UnwrappedLineParser::parseBlock(bool MustBeDeclaration, bool AddLevel, void UnwrappedLineParser::parseBlock(bool MustBeDeclaration, bool AddLevel,
bool MunchSemi) { bool MunchSemi) {
assert(FormatTok->isOneOf(tok::l_brace, TT_MacroBlockBegin) && assert(FormatTok->isOneOf(tok::l_brace, TT_MacroBlockBegin) &&
"'{' or macro block token expected"); "'{' or macro block token expected");
const bool MacroBlock = FormatTok->is(TT_MacroBlockBegin); const bool MacroBlock = FormatTok->is(TT_MacroBlockBegin);
FormatTok->BlockKind = BK_Block; FormatTok->BlockKind = BK_Block;
unsigned InitialLevel = Line->Level; unsigned InitialLevel = Line->Level;
nextToken(); nextToken(/*LevelOfCommentLinesAfter=*/
InitialLevel + AddLevel);
if (MacroBlock && FormatTok->is(tok::l_paren)) if (MacroBlock && FormatTok->is(tok::l_paren))
parseParens(); parseParens();
addUnwrappedLine(); addUnwrappedLine();
size_t OpeningLineIndex = CurrentLines->empty() size_t OpeningLineIndex = CurrentLines->empty()
? (UnwrappedLine::kInvalidIndex) ? (UnwrappedLine::kInvalidIndex)
: (CurrentLines->size() - 1); : (CurrentLines->size() - 1);
Show All 9 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)); // Munch the closing brace.
Line->Level = InitialLevel; nextToken(/*LevelOfCommentLinesAfter=*/InitialLevel);
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->MatchingOpeningBlockLineIndex = OpeningLineIndex; Line->MatchingOpeningBlockLineIndex = OpeningLineIndex;
Line->Level = InitialLevel;
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;
} }
} }
static bool isGoogScope(const UnwrappedLine &Line) { static bool isGoogScope(const UnwrappedLine &Line) {
▲ Show 20 Lines • Show All 1,776 Lines • ▼ Show 20 Lines if (isOnNewLine(**I) && JustComments && !(*I)->ContinuesLineCommentSection)
addUnwrappedLine(); addUnwrappedLine();
pushToken(*I); pushToken(*I);
} }
if (NewlineBeforeNext && JustComments) if (NewlineBeforeNext && JustComments)
addUnwrappedLine(); addUnwrappedLine();
CommentsBeforeNextToken.clear(); CommentsBeforeNextToken.clear();
} }
void UnwrappedLineParser::nextToken() { void UnwrappedLineParser::nextToken(
llvm::Optional<unsigned> LevelOfCommentLinesAfter) {
if (eof()) if (eof())
return; return;
flushComments(isOnNewLine(*FormatTok)); flushComments(isOnNewLine(*FormatTok));
pushToken(FormatTok); pushToken(FormatTok);
if (Style.Language != FormatStyle::LK_JavaScript) if (Style.Language != FormatStyle::LK_JavaScript)
readToken(); readToken(LevelOfCommentLinesAfter);
else else
readTokenWithJavaScriptASI(); readTokenWithJavaScriptASI();
} }
const FormatToken *UnwrappedLineParser::getPreviousToken() { const FormatToken *UnwrappedLineParser::getPreviousToken() {
// FIXME: This is a dirty way to access the previous token. Find a better // FIXME: This is a dirty way to access the previous token. Find a better
// solution. // solution.
if (!Line || Line->Tokens.empty()) if (!Line || Line->Tokens.empty())
▲ Show 20 Lines • Show All 52 Lines • ▼ Show 20 Lines for (unsigned i = 0, e = Comments.size(); i < e; ++i) {
if (ShouldPushCommentsInCurrentLine) { if (ShouldPushCommentsInCurrentLine) {
pushToken(FormatTok); pushToken(FormatTok);
} else { } else {
CommentsBeforeNextToken.push_back(FormatTok); CommentsBeforeNextToken.push_back(FormatTok);
} }
} }
} }
void UnwrappedLineParser::readToken() { void UnwrappedLineParser::readToken(
llvm::Optional<unsigned> LevelOfCommentLinesAfter) {
SmallVector<FormatToken *, 1> Comments; SmallVector<FormatToken *, 1> Comments;
do { do {
FormatTok = Tokens->getNextToken(); FormatTok = Tokens->getNextToken();
assert(FormatTok); assert(FormatTok);
while (!Line->InPPDirective && FormatTok->Tok.is(tok::hash) && while (!Line->InPPDirective && FormatTok->Tok.is(tok::hash) &&
(FormatTok->HasUnescapedNewline || FormatTok->IsFirst)) { (FormatTok->HasUnescapedNewline || FormatTok->IsFirst)) {
distributeComments(Comments, FormatTok); distributeComments(Comments, FormatTok);
Comments.clear(); Comments.clear();
// If there is an unfinished unwrapped line, we flush the preprocessor // If there is an unfinished unwrapped line, we flush the preprocessor
// directives only after that unwrapped line was finished later. // directives only after that unwrapped line was finished later.
bool SwitchToPreprocessorLines = !Line->Tokens.empty(); bool SwitchToPreprocessorLines = !Line->Tokens.empty();
ScopedLineState BlockState(*this, SwitchToPreprocessorLines); ScopedLineState BlockState(*this, SwitchToPreprocessorLines);
if (LevelOfCommentLinesAfter)
Line->Level = *LevelOfCommentLinesAfter;
// Comments stored before the preprocessor directive need to be output // Comments stored before the preprocessor directive need to be output
// before the preprocessor directive, at the same level as the // before the preprocessor directive, at the same level as the
// preprocessor directive, as we consider them to apply to the directive. // preprocessor directive, as we consider them to apply to the directive.
flushComments(isOnNewLine(*FormatTok)); flushComments(isOnNewLine(*FormatTok));
parsePPDirective(); parsePPDirective();
} }
while (FormatTok->Type == TT_ConflictStart || while (FormatTok->Type == TT_ConflictStart ||
FormatTok->Type == TT_ConflictEnd || FormatTok->Type == TT_ConflictEnd ||
Show All 40 Lines

unittests/Format/FormatTestComments.cpp

Show First 20 Lines • Show All 849 Lines • ▼ Show 20 Lines EXPECT_EQ("int f(int i) {\n"
" ++i;\n" " ++i;\n"
" }\n" " }\n"
" // comment\n" " // comment\n"
"#ifdef A\n" "#ifdef A\n"
"int j;\n" "int j;\n"
"#endif\n" "#endif\n"
"}")); "}"));
EXPECT_EQ("int f(int i) {\n"
" if (true) {\n"
" i++;\n"
" } else {\n"
" // comment in else\n"
"#ifdef A\n"
" j++;\n"
"#endif\n"
" }\n"
"}",
format("int f(int i) {\n"
" if (true) {\n"
" i++;\n"
" } else {\n"
" // comment in else\n"
"#ifdef A\n"
" j++;\n"
"#endif\n"
" }\n"
"}"));
EXPECT_EQ("int f(int i) {\n"
" if (true) {\n"
" i++;\n"
" } else {\n"
" /* comment in else */\n"
"#ifdef A\n"
" j++;\n"
"#endif\n"
" }\n"
"}",
format("int f(int i) {\n"
" if (true) {\n"
" i++;\n"
" } else {\n"
" /* comment in else */\n"
"#ifdef A\n"
" j++;\n"
"#endif\n"
" }\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"
▲ Show 20 Lines • Show All 41 Lines • ▼ Show 20 Lines EXPECT_EQ("int f(int i) {\n"
" }\n" " }\n"
" // comment\n" " // comment\n"
"\n" "\n"
"#ifdef A\n" "#ifdef A\n"
" int j;\n" " int j;\n"
"#endif\n" "#endif\n"
"}")); "}"));
EXPECT_EQ("int f(int i) {\n"
" if (true) {\n"
" i++;\n"
" } else {\n"
" // comment in else\n"
"\n"
"#ifdef A\n"
" j++;\n"
"#endif\n"
" }\n"
"}",
format("int f(int i) {\n"
" if (true) {\n"
" i++;\n"
" } else {\n"
"// comment in else\n"
"\n"
"#ifdef A\n"
" j++;\n"
"#endif\n"
" }\n"
"}"));
EXPECT_EQ("int f(int i) {\n"
" if (true) {\n"
" i++;\n"
" } else {\n"
" /* comment in else */\n"
"\n"
"#ifdef A\n"
" j++;\n"
"#endif\n"
" }\n"
"}",
format("int f(int i) {\n"
" if (true) {\n"
" i++;\n"
" } else {\n"
"/* comment in else */\n"
"\n"
"#ifdef A\n"
" j++;\n"
"#endif\n"
" }\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 and there is no newline between the comment
// and 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"
Show All 15 Lines EXPECT_EQ("int f(int i) {\n"
" if (true) {\n" " if (true) {\n"
" ++i;\n" " ++i;\n"
" }\n" " }\n"
"// comment\n" "// comment\n"
"#ifdef A\n" "#ifdef A\n"
" int j;\n" " int j;\n"
"#endif\n" "#endif\n"
"}")); "}"));
EXPECT_EQ("int f(int i) {\n"
" if (true) {\n"
" i++;\n"
" } else {\n"
"// comment in else\n"
"#ifdef A\n"
" j++;\n"
"#endif\n"
" }\n"
"}",
format("int f(int i) {\n"
" if (true) {\n"
" i++;\n"
" } else {\n"
" // comment in else\n"
" #ifdef A\n"
" j++;\n"
"#endif\n"
" }\n"
"}"));
EXPECT_EQ("int f(int i) {\n"
" if (true) {\n"
" i++;\n"
" } else {\n"
"/* comment in else */\n"
"#ifdef A\n"
" j++;\n"
"#endif\n"
" }\n"
"}",
format("int f(int i) {\n"
" if (true) {\n"
" i++;\n"
" } else {\n"
" /* comment in else */\n"
" #ifdef A\n"
" j++;\n"
"#endif\n"
" }\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,688 Lines • ▼ Show 20 Lines EXPECT_EQ("/*\n"
" * long */", " * long */",
format("/*\n" format("/*\n"
" * long long long long\n" " * long long long long\n"
" * long\n" " * long\n"
" *\n" " *\n"
"* long */", "* long */",
getLLVMStyleWithColumns(20))); getLLVMStyleWithColumns(20)));
} }
TEST_F(FormatTestComments, TODO) {
EXPECT_EQ("int f(int i, int j) {\n"
" if (true) {\n"
" i++;\n"
" } else {\n"
" // comment in else\n"
"#ifdef A\n"
" j++;\n"
"#endif\n"
" }\n"
"}",
format("int f(int i, int j) {\n"
" if (true) {\n"
" i++;\n"
" } else {\n"
" // comment in else\n"
"#ifdef A\n"
" j++;\n"
"#endif\n"
" }\n"
"}",
getLLVMStyle()));
}
} // end namespace } // end namespace
} // end namespace format } // end namespace format
} // end namespace clang } // end namespace clang