On Sun, May 26, 2013 at 7:45 AM, Nico Weber <[email protected]> wrote:
> On Fri, Apr 12, 2013 at 7:13 AM, Manuel Klimek <[email protected]> wrote: > >> Author: klimek >> Date: Fri Apr 12 09:13:36 2013 >> New Revision: 179379 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=179379&view=rev >> Log: >> 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 <eof> >> - 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 >> >> Modified: >> cfe/trunk/lib/Format/Format.cpp >> cfe/trunk/lib/Format/UnwrappedLineParser.cpp >> cfe/trunk/lib/Format/UnwrappedLineParser.h >> cfe/trunk/unittests/Format/FormatTest.cpp >> >> Modified: cfe/trunk/lib/Format/Format.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/Format.cpp?rev=179379&r1=179378&r2=179379&view=diff >> >> ============================================================================== >> --- cfe/trunk/lib/Format/Format.cpp (original) >> +++ cfe/trunk/lib/Format/Format.cpp Fri Apr 12 09:13:36 2013 >> @@ -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<int>(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<CharSourceRange> Ranges; >> std::vector<AnnotatedLine> AnnotatedLines; >> - bool StructuralError; >> }; >> >> tooling::Replacements >> >> Modified: cfe/trunk/lib/Format/UnwrappedLineParser.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/UnwrappedLineParser.cpp?rev=179379&r1=179378&r2=179379&view=diff >> >> ============================================================================== >> --- cfe/trunk/lib/Format/UnwrappedLineParser.cpp (original) >> +++ cfe/trunk/lib/Format/UnwrappedLineParser.cpp Fri Apr 12 09:13:36 2013 >> @@ -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<UnwrappedLine>::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(boo >> 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(boo >> >> 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) { >> >> Modified: cfe/trunk/lib/Format/UnwrappedLineParser.h >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/UnwrappedLineParser.h?rev=179379&r1=179378&r2=179379&view=diff >> >> ============================================================================== >> --- cfe/trunk/lib/Format/UnwrappedLineParser.h (original) >> +++ cfe/trunk/lib/Format/UnwrappedLineParser.h Fri Apr 12 09:13:36 2013 >> @@ -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<bool> 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; >> >> Modified: cfe/trunk/unittests/Format/FormatTest.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=179379&r1=179378&r2=179379&view=diff >> >> ============================================================================== >> --- cfe/trunk/unittests/Format/FormatTest.cpp (original) >> +++ cfe/trunk/unittests/Format/FormatTest.cpp Fri Apr 12 09:13:36 2013 >> @@ -430,6 +430,7 @@ TEST_F(FormatTest, FormatsSwitchStatemen >> 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. >> > > Any progress on this? This regressed working functionality. > Unfortunately I have no idea about Obj-C and a short search didn't get me a BNS that explained what can be done here... One question is: do we really need to special case every identifier with an "@" in front of it, or is there a mode where "@{" for example gets output as a single token (or is there a fundamental concept for @ in the language that I need to understand first?) Thx, /Manuel > > >> + 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 }"); >> >> >> _______________________________________________ >> cfe-commits mailing list >> [email protected] >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
