From 67d080dafa74c7d126da522fec333a6e52a5ae35 Mon Sep 17 00:00:00 2001 From: Manuel Klimek Date: Fri, 12 Apr 2013 14:13:36 +0000 Subject: [PATCH] Revamps structural error detection / handling. Previously we'd only detect structural errors on the very first level. This leads to incorrectly balanced braces not being discovered, and thus incorrect indentation. This change fixes the problem by: - changing the parser to use an error state that can be detected anywhere inside the productions, for example if we get an eof on SOME_MACRO({ some block - previously we'd never break lines when we discovered a structural error; now we break even in the case of a structural error if there are two unwrapped lines within the same line; thus, void f() { while (true) { g(); y(); } } will still be re-formatted, even if there's missing braces somewhere in the file - still exclude macro definitions from generating structural error; macro definitions are inbalanced snippets git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@179379 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/Format/Format.cpp | 19 ++++++----- lib/Format/UnwrappedLineParser.cpp | 55 +++++++++++++++--------------- lib/Format/UnwrappedLineParser.h | 10 ++++-- unittests/Format/FormatTest.cpp | 15 ++++++-- 4 files changed, 57 insertions(+), 42 deletions(-) diff --git a/lib/Format/Format.cpp b/lib/Format/Format.cpp index e956f69875..d0dfdceddc 100644 --- a/lib/Format/Format.cpp +++ b/lib/Format/Format.cpp @@ -459,7 +459,7 @@ public: UnwrappedLineFormatter(const FormatStyle &Style, SourceManager &SourceMgr, const AnnotatedLine &Line, unsigned FirstIndent, const AnnotatedToken &RootToken, - WhitespaceManager &Whitespaces, bool StructuralError) + WhitespaceManager &Whitespaces) : Style(Style), SourceMgr(SourceMgr), Line(Line), FirstIndent(FirstIndent), RootToken(RootToken), Whitespaces(Whitespaces), Count(0) {} @@ -1381,7 +1381,7 @@ public: tooling::Replacements format() { LexerBasedFormatTokenSource Tokens(Lex, SourceMgr); UnwrappedLineParser Parser(Diag, Style, Tokens, *this); - StructuralError = Parser.parse(); + bool StructuralError = Parser.parse(); unsigned PreviousEndOfLineColumn = 0; TokenAnnotator Annotator(Style, SourceMgr, Lex, Tokens.getIdentTable().get("in")); @@ -1431,17 +1431,19 @@ public: unsigned Indent = LevelIndent; if (static_cast(Indent) + Offset >= 0) Indent += Offset; - if (!FirstTok.WhiteSpaceStart.isValid() || StructuralError) { - Indent = LevelIndent = - SourceMgr.getSpellingColumnNumber(FirstTok.Tok.getLocation()) - 1; - } else { + if (FirstTok.WhiteSpaceStart.isValid() && + // Insert a break even if there is a structural error in case where + // we break apart a line consisting of multiple unwrapped lines. + (FirstTok.NewlinesBefore == 0 || !StructuralError)) { formatFirstToken(TheLine.First, PreviousLineLastToken, Indent, TheLine.InPPDirective, PreviousEndOfLineColumn); + } else { + Indent = LevelIndent = + SourceMgr.getSpellingColumnNumber(FirstTok.Tok.getLocation()) - 1; } tryFitMultipleLinesInOne(Indent, I, E); UnwrappedLineFormatter Formatter(Style, SourceMgr, TheLine, Indent, - TheLine.First, Whitespaces, - StructuralError); + TheLine.First, Whitespaces); PreviousEndOfLineColumn = Formatter.format(I + 1 != E ? &*(I + 1) : NULL); IndentForLevel[TheLine.Level] = LevelIndent; @@ -1742,7 +1744,6 @@ private: WhitespaceManager Whitespaces; std::vector Ranges; std::vector AnnotatedLines; - bool StructuralError; }; tooling::Replacements diff --git a/lib/Format/UnwrappedLineParser.cpp b/lib/Format/UnwrappedLineParser.cpp index cf2ef718e9..722af5d2b7 100644 --- a/lib/Format/UnwrappedLineParser.cpp +++ b/lib/Format/UnwrappedLineParser.cpp @@ -45,9 +45,11 @@ private: class ScopedMacroState : public FormatTokenSource { public: ScopedMacroState(UnwrappedLine &Line, FormatTokenSource *&TokenSource, - FormatToken &ResetToken) + FormatToken &ResetToken, bool &StructuralError) : Line(Line), TokenSource(TokenSource), ResetToken(ResetToken), - PreviousLineLevel(Line.Level), PreviousTokenSource(TokenSource) { + PreviousLineLevel(Line.Level), PreviousTokenSource(TokenSource), + StructuralError(StructuralError), + PreviousStructuralError(StructuralError) { TokenSource = this; Line.Level = 0; Line.InPPDirective = true; @@ -58,6 +60,7 @@ public: ResetToken = Token; Line.InPPDirective = false; Line.Level = PreviousLineLevel; + StructuralError = PreviousStructuralError; } virtual FormatToken getNextToken() { @@ -85,6 +88,8 @@ private: FormatToken &ResetToken; unsigned PreviousLineLevel; FormatTokenSource *PreviousTokenSource; + bool &StructuralError; + bool PreviousStructuralError; FormatToken Token; }; @@ -124,13 +129,13 @@ UnwrappedLineParser::UnwrappedLineParser( clang::DiagnosticsEngine &Diag, const FormatStyle &Style, FormatTokenSource &Tokens, UnwrappedLineConsumer &Callback) : Line(new UnwrappedLine), MustBreakBeforeNextToken(false), - CurrentLines(&Lines), Diag(Diag), Style(Style), Tokens(&Tokens), - Callback(Callback) {} + CurrentLines(&Lines), StructuralError(false), Diag(Diag), Style(Style), + Tokens(&Tokens), Callback(Callback) {} bool UnwrappedLineParser::parse() { DEBUG(llvm::dbgs() << "----\n"); readToken(); - bool Error = parseFile(); + parseFile(); for (std::vector::iterator I = Lines.begin(), E = Lines.end(); I != E; ++I) { Callback.consumeUnwrappedLine(*I); @@ -139,23 +144,20 @@ bool UnwrappedLineParser::parse() { // Create line with eof token. pushToken(FormatTok); Callback.consumeUnwrappedLine(*Line); - - return Error; + return StructuralError; } -bool UnwrappedLineParser::parseFile() { +void UnwrappedLineParser::parseFile() { ScopedDeclarationState DeclarationState( *Line, DeclarationScopeStack, /*MustBeDeclaration=*/ !Line->InPPDirective); - bool Error = parseLevel(/*HasOpeningBrace=*/ false); + parseLevel(/*HasOpeningBrace=*/ false); // Make sure to format the remaining tokens. flushComments(true); addUnwrappedLine(); - return Error; } -bool UnwrappedLineParser::parseLevel(bool HasOpeningBrace) { - bool Error = false; +void UnwrappedLineParser::parseLevel(bool HasOpeningBrace) { do { switch (FormatTok.Tok.getKind()) { case tok::comment: @@ -165,30 +167,27 @@ bool UnwrappedLineParser::parseLevel(bool HasOpeningBrace) { case tok::l_brace: // FIXME: Add parameter whether this can happen - if this happens, we must // be in a non-declaration context. - Error |= parseBlock(/*MustBeDeclaration=*/ false); + parseBlock(/*MustBeDeclaration=*/ false); addUnwrappedLine(); break; case tok::r_brace: - if (HasOpeningBrace) { - return false; - } else { - Diag.Report(FormatTok.Tok.getLocation(), - Diag.getCustomDiagID(clang::DiagnosticsEngine::Error, - "unexpected '}'")); - Error = true; - nextToken(); - addUnwrappedLine(); - } + if (HasOpeningBrace) + return; + Diag.Report(FormatTok.Tok.getLocation(), + Diag.getCustomDiagID(clang::DiagnosticsEngine::Error, + "unexpected '}'")); + StructuralError = true; + nextToken(); + addUnwrappedLine(); break; default: parseStructuralElement(); break; } } while (!eof()); - return Error; } -bool UnwrappedLineParser::parseBlock(bool MustBeDeclaration, +void UnwrappedLineParser::parseBlock(bool MustBeDeclaration, unsigned AddLevels) { assert(FormatTok.Tok.is(tok::l_brace) && "'{' expected"); nextToken(); @@ -202,17 +201,17 @@ bool UnwrappedLineParser::parseBlock(bool MustBeDeclaration, if (!FormatTok.Tok.is(tok::r_brace)) { Line->Level -= AddLevels; - return true; + StructuralError = true; + return; } nextToken(); // Munch the closing brace. Line->Level -= AddLevels; - return false; } void UnwrappedLineParser::parsePPDirective() { assert(FormatTok.Tok.is(tok::hash) && "'#' expected"); - ScopedMacroState MacroState(*Line, Tokens, FormatTok); + ScopedMacroState MacroState(*Line, Tokens, FormatTok, StructuralError); nextToken(); if (FormatTok.Tok.getIdentifierInfo() == NULL) { diff --git a/lib/Format/UnwrappedLineParser.h b/lib/Format/UnwrappedLineParser.h index f4fecc5ef0..1326d500e2 100644 --- a/lib/Format/UnwrappedLineParser.h +++ b/lib/Format/UnwrappedLineParser.h @@ -125,9 +125,9 @@ public: bool parse(); private: - bool parseFile(); - bool parseLevel(bool HasOpeningBrace); - bool parseBlock(bool MustBeDeclaration, unsigned AddLevels = 1); + void parseFile(); + void parseLevel(bool HasOpeningBrace); + void parseBlock(bool MustBeDeclaration, unsigned AddLevels = 1); void parsePPDirective(); void parsePPDefine(); void parsePPUnknown(); @@ -187,6 +187,10 @@ private: // whether we are in a compound statement or not. std::vector DeclarationScopeStack; + // Will be true if we encounter an error that leads to possibily incorrect + // indentation levels. + bool StructuralError; + clang::DiagnosticsEngine &Diag; const FormatStyle &Style; FormatTokenSource *Tokens; diff --git a/unittests/Format/FormatTest.cpp b/unittests/Format/FormatTest.cpp index 91b8f81080..8dadd032ab 100644 --- a/unittests/Format/FormatTest.cpp +++ b/unittests/Format/FormatTest.cpp @@ -430,6 +430,7 @@ TEST_F(FormatTest, FormatsSwitchStatement) { verifyFormat("switch (x) {\n" "default: {\n" " // Do nothing.\n" + "}\n" "}"); verifyFormat("switch (x) {\n" "// comment\n" @@ -3563,8 +3564,18 @@ TEST_F(FormatTest, ObjCLiterals) { verifyFormat("@{ @\"one\" : @1 }"); verifyFormat("return @{ @\"one\" : @1 };"); verifyFormat("@{ @\"one\" : @1, }"); - verifyFormat("@{ @\"one\" : @{ @2 : @1 } }"); - verifyFormat("@{ @\"one\" : @{ @2 : @1 }, }"); + + // FIXME: Breaking in cases where we think there's a structural error + // showed that we're incorrectly parsing this code. We need to fix the + // parsing here. + verifyFormat("@{ @\"one\" : @\n" + "{ @2 : @1 }\n" + "}"); + verifyFormat("@{ @\"one\" : @\n" + "{ @2 : @1 }\n" + ",\n" + "}"); + verifyFormat("@{ 1 > 2 ? @\"one\" : @\"two\" : 1 > 2 ? @1 : @2 }"); verifyFormat("[self setDict:@{}"); verifyFormat("[self setDict:@{ @1 : @2 }"); -- 2.40.0