<div dir="ltr">On Sun, May 26, 2013 at 7:45 AM, Nico Weber <span dir="ltr"><<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div class="h5">On Fri, Apr 12, 2013 at 7:13 AM, Manuel Klimek <span dir="ltr"><<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>></span> wrote:<br>
</div></div><div class="gmail_extra"><div class="gmail_quote"><div><div class="h5">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: klimek<br>
Date: Fri Apr 12 09:13:36 2013<br>
New Revision: 179379<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=179379&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=179379&view=rev</a><br>
Log:<br>
Revamps structural error detection / handling.<br>
<br>
Previously we'd only detect structural errors on the very first level.<br>
This leads to incorrectly balanced braces not being discovered, and thus<br>
incorrect indentation.<br>
<br>
This change fixes the problem by:<br>
- changing the parser to use an error state that can be detected<br>
anywhere inside the productions, for example if we get an eof on<br>
SOME_MACRO({ some block <eof><br>
- previously we'd never break lines when we discovered a structural<br>
error; now we break even in the case of a structural error if there<br>
are two unwrapped lines within the same line; thus,<br>
void f() { while (true) { g(); y(); } }<br>
will still be re-formatted, even if there's missing braces somewhere<br>
in the file<br>
- still exclude macro definitions from generating structural error;<br>
macro definitions are inbalanced snippets<br>
<br>
Modified:<br>
cfe/trunk/lib/Format/Format.cpp<br>
cfe/trunk/lib/Format/UnwrappedLineParser.cpp<br>
cfe/trunk/lib/Format/UnwrappedLineParser.h<br>
cfe/trunk/unittests/Format/FormatTest.cpp<br>
<br>
Modified: cfe/trunk/lib/Format/Format.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/Format.cpp?rev=179379&r1=179378&r2=179379&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/Format.cpp?rev=179379&r1=179378&r2=179379&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Format/Format.cpp (original)<br>
+++ cfe/trunk/lib/Format/Format.cpp Fri Apr 12 09:13:36 2013<br>
@@ -459,7 +459,7 @@ public:<br>
UnwrappedLineFormatter(const FormatStyle &Style, SourceManager &SourceMgr,<br>
const AnnotatedLine &Line, unsigned FirstIndent,<br>
const AnnotatedToken &RootToken,<br>
- WhitespaceManager &Whitespaces, bool StructuralError)<br>
+ WhitespaceManager &Whitespaces)<br>
: Style(Style), SourceMgr(SourceMgr), Line(Line),<br>
FirstIndent(FirstIndent), RootToken(RootToken),<br>
Whitespaces(Whitespaces), Count(0) {}<br>
@@ -1381,7 +1381,7 @@ public:<br>
tooling::Replacements format() {<br>
LexerBasedFormatTokenSource Tokens(Lex, SourceMgr);<br>
UnwrappedLineParser Parser(Diag, Style, Tokens, *this);<br>
- StructuralError = Parser.parse();<br>
+ bool StructuralError = Parser.parse();<br>
unsigned PreviousEndOfLineColumn = 0;<br>
TokenAnnotator Annotator(Style, SourceMgr, Lex,<br>
Tokens.getIdentTable().get("in"));<br>
@@ -1431,17 +1431,19 @@ public:<br>
unsigned Indent = LevelIndent;<br>
if (static_cast<int>(Indent) + Offset >= 0)<br>
Indent += Offset;<br>
- if (!FirstTok.WhiteSpaceStart.isValid() || StructuralError) {<br>
- Indent = LevelIndent =<br>
- SourceMgr.getSpellingColumnNumber(FirstTok.Tok.getLocation()) - 1;<br>
- } else {<br>
+ if (FirstTok.WhiteSpaceStart.isValid() &&<br>
+ // Insert a break even if there is a structural error in case where<br>
+ // we break apart a line consisting of multiple unwrapped lines.<br>
+ (FirstTok.NewlinesBefore == 0 || !StructuralError)) {<br>
formatFirstToken(TheLine.First, PreviousLineLastToken, Indent,<br>
TheLine.InPPDirective, PreviousEndOfLineColumn);<br>
+ } else {<br>
+ Indent = LevelIndent =<br>
+ SourceMgr.getSpellingColumnNumber(FirstTok.Tok.getLocation()) - 1;<br>
}<br>
tryFitMultipleLinesInOne(Indent, I, E);<br>
UnwrappedLineFormatter Formatter(Style, SourceMgr, TheLine, Indent,<br>
- TheLine.First, Whitespaces,<br>
- StructuralError);<br>
+ TheLine.First, Whitespaces);<br>
PreviousEndOfLineColumn =<br>
Formatter.format(I + 1 != E ? &*(I + 1) : NULL);<br>
IndentForLevel[TheLine.Level] = LevelIndent;<br>
@@ -1742,7 +1744,6 @@ private:<br>
WhitespaceManager Whitespaces;<br>
std::vector<CharSourceRange> Ranges;<br>
std::vector<AnnotatedLine> AnnotatedLines;<br>
- bool StructuralError;<br>
};<br>
<br>
tooling::Replacements<br>
<br>
Modified: cfe/trunk/lib/Format/UnwrappedLineParser.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/UnwrappedLineParser.cpp?rev=179379&r1=179378&r2=179379&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/UnwrappedLineParser.cpp?rev=179379&r1=179378&r2=179379&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Format/UnwrappedLineParser.cpp (original)<br>
+++ cfe/trunk/lib/Format/UnwrappedLineParser.cpp Fri Apr 12 09:13:36 2013<br>
@@ -45,9 +45,11 @@ private:<br>
class ScopedMacroState : public FormatTokenSource {<br>
public:<br>
ScopedMacroState(UnwrappedLine &Line, FormatTokenSource *&TokenSource,<br>
- FormatToken &ResetToken)<br>
+ FormatToken &ResetToken, bool &StructuralError)<br>
: Line(Line), TokenSource(TokenSource), ResetToken(ResetToken),<br>
- PreviousLineLevel(Line.Level), PreviousTokenSource(TokenSource) {<br>
+ PreviousLineLevel(Line.Level), PreviousTokenSource(TokenSource),<br>
+ StructuralError(StructuralError),<br>
+ PreviousStructuralError(StructuralError) {<br>
TokenSource = this;<br>
Line.Level = 0;<br>
Line.InPPDirective = true;<br>
@@ -58,6 +60,7 @@ public:<br>
ResetToken = Token;<br>
Line.InPPDirective = false;<br>
Line.Level = PreviousLineLevel;<br>
+ StructuralError = PreviousStructuralError;<br>
}<br>
<br>
virtual FormatToken getNextToken() {<br>
@@ -85,6 +88,8 @@ private:<br>
FormatToken &ResetToken;<br>
unsigned PreviousLineLevel;<br>
FormatTokenSource *PreviousTokenSource;<br>
+ bool &StructuralError;<br>
+ bool PreviousStructuralError;<br>
<br>
FormatToken Token;<br>
};<br>
@@ -124,13 +129,13 @@ UnwrappedLineParser::UnwrappedLineParser<br>
clang::DiagnosticsEngine &Diag, const FormatStyle &Style,<br>
FormatTokenSource &Tokens, UnwrappedLineConsumer &Callback)<br>
: Line(new UnwrappedLine), MustBreakBeforeNextToken(false),<br>
- CurrentLines(&Lines), Diag(Diag), Style(Style), Tokens(&Tokens),<br>
- Callback(Callback) {}<br>
+ CurrentLines(&Lines), StructuralError(false), Diag(Diag), Style(Style),<br>
+ Tokens(&Tokens), Callback(Callback) {}<br>
<br>
bool UnwrappedLineParser::parse() {<br>
DEBUG(llvm::dbgs() << "----\n");<br>
readToken();<br>
- bool Error = parseFile();<br>
+ parseFile();<br>
for (std::vector<UnwrappedLine>::iterator I = Lines.begin(), E = Lines.end();<br>
I != E; ++I) {<br>
Callback.consumeUnwrappedLine(*I);<br>
@@ -139,23 +144,20 @@ bool UnwrappedLineParser::parse() {<br>
// Create line with eof token.<br>
pushToken(FormatTok);<br>
Callback.consumeUnwrappedLine(*Line);<br>
-<br>
- return Error;<br>
+ return StructuralError;<br>
}<br>
<br>
-bool UnwrappedLineParser::parseFile() {<br>
+void UnwrappedLineParser::parseFile() {<br>
ScopedDeclarationState DeclarationState(<br>
*Line, DeclarationScopeStack,<br>
/*MustBeDeclaration=*/ !Line->InPPDirective);<br>
- bool Error = parseLevel(/*HasOpeningBrace=*/ false);<br>
+ parseLevel(/*HasOpeningBrace=*/ false);<br>
// Make sure to format the remaining tokens.<br>
flushComments(true);<br>
addUnwrappedLine();<br>
- return Error;<br>
}<br>
<br>
-bool UnwrappedLineParser::parseLevel(bool HasOpeningBrace) {<br>
- bool Error = false;<br>
+void UnwrappedLineParser::parseLevel(bool HasOpeningBrace) {<br>
do {<br>
switch (FormatTok.Tok.getKind()) {<br>
case tok::comment:<br>
@@ -165,30 +167,27 @@ bool UnwrappedLineParser::parseLevel(boo<br>
case tok::l_brace:<br>
// FIXME: Add parameter whether this can happen - if this happens, we must<br>
// be in a non-declaration context.<br>
- Error |= parseBlock(/*MustBeDeclaration=*/ false);<br>
+ parseBlock(/*MustBeDeclaration=*/ false);<br>
addUnwrappedLine();<br>
break;<br>
case tok::r_brace:<br>
- if (HasOpeningBrace) {<br>
- return false;<br>
- } else {<br>
- Diag.Report(FormatTok.Tok.getLocation(),<br>
- Diag.getCustomDiagID(clang::DiagnosticsEngine::Error,<br>
- "unexpected '}'"));<br>
- Error = true;<br>
- nextToken();<br>
- addUnwrappedLine();<br>
- }<br>
+ if (HasOpeningBrace)<br>
+ return;<br>
+ Diag.Report(FormatTok.Tok.getLocation(),<br>
+ Diag.getCustomDiagID(clang::DiagnosticsEngine::Error,<br>
+ "unexpected '}'"));<br>
+ StructuralError = true;<br>
+ nextToken();<br>
+ addUnwrappedLine();<br>
break;<br>
default:<br>
parseStructuralElement();<br>
break;<br>
}<br>
} while (!eof());<br>
- return Error;<br>
}<br>
<br>
-bool UnwrappedLineParser::parseBlock(bool MustBeDeclaration,<br>
+void UnwrappedLineParser::parseBlock(bool MustBeDeclaration,<br>
unsigned AddLevels) {<br>
assert(<a href="http://FormatTok.Tok.is" target="_blank">FormatTok.Tok.is</a>(tok::l_brace) && "'{' expected");<br>
nextToken();<br>
@@ -202,17 +201,17 @@ bool UnwrappedLineParser::parseBlock(boo<br>
<br>
if (!<a href="http://FormatTok.Tok.is" target="_blank">FormatTok.Tok.is</a>(tok::r_brace)) {<br>
Line->Level -= AddLevels;<br>
- return true;<br>
+ StructuralError = true;<br>
+ return;<br>
}<br>
<br>
nextToken(); // Munch the closing brace.<br>
Line->Level -= AddLevels;<br>
- return false;<br>
}<br>
<br>
void UnwrappedLineParser::parsePPDirective() {<br>
assert(<a href="http://FormatTok.Tok.is" target="_blank">FormatTok.Tok.is</a>(tok::hash) && "'#' expected");<br>
- ScopedMacroState MacroState(*Line, Tokens, FormatTok);<br>
+ ScopedMacroState MacroState(*Line, Tokens, FormatTok, StructuralError);<br>
nextToken();<br>
<br>
if (FormatTok.Tok.getIdentifierInfo() == NULL) {<br>
<br>
Modified: cfe/trunk/lib/Format/UnwrappedLineParser.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/UnwrappedLineParser.h?rev=179379&r1=179378&r2=179379&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/UnwrappedLineParser.h?rev=179379&r1=179378&r2=179379&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Format/UnwrappedLineParser.h (original)<br>
+++ cfe/trunk/lib/Format/UnwrappedLineParser.h Fri Apr 12 09:13:36 2013<br>
@@ -125,9 +125,9 @@ public:<br>
bool parse();<br>
<br>
private:<br>
- bool parseFile();<br>
- bool parseLevel(bool HasOpeningBrace);<br>
- bool parseBlock(bool MustBeDeclaration, unsigned AddLevels = 1);<br>
+ void parseFile();<br>
+ void parseLevel(bool HasOpeningBrace);<br>
+ void parseBlock(bool MustBeDeclaration, unsigned AddLevels = 1);<br>
void parsePPDirective();<br>
void parsePPDefine();<br>
void parsePPUnknown();<br>
@@ -187,6 +187,10 @@ private:<br>
// whether we are in a compound statement or not.<br>
std::vector<bool> DeclarationScopeStack;<br>
<br>
+ // Will be true if we encounter an error that leads to possibily incorrect<br>
+ // indentation levels.<br>
+ bool StructuralError;<br>
+<br>
clang::DiagnosticsEngine &Diag;<br>
const FormatStyle &Style;<br>
FormatTokenSource *Tokens;<br>
<br>
Modified: cfe/trunk/unittests/Format/FormatTest.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=179379&r1=179378&r2=179379&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=179379&r1=179378&r2=179379&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/unittests/Format/FormatTest.cpp (original)<br>
+++ cfe/trunk/unittests/Format/FormatTest.cpp Fri Apr 12 09:13:36 2013<br>
@@ -430,6 +430,7 @@ TEST_F(FormatTest, FormatsSwitchStatemen<br>
verifyFormat("switch (x) {\n"<br>
"default: {\n"<br>
" // Do nothing.\n"<br>
+ "}\n"<br>
"}");<br>
verifyFormat("switch (x) {\n"<br>
"// comment\n"<br>
@@ -3563,8 +3564,18 @@ TEST_F(FormatTest, ObjCLiterals) {<br>
verifyFormat("@{ @\"one\" : @1 }");<br>
verifyFormat("return @{ @\"one\" : @1 };");<br>
verifyFormat("@{ @\"one\" : @1, }");<br>
- verifyFormat("@{ @\"one\" : @{ @2 : @1 } }");<br>
- verifyFormat("@{ @\"one\" : @{ @2 : @1 }, }");<br>
+<br>
+ // FIXME: Breaking in cases where we think there's a structural error<br>
+ // showed that we're incorrectly parsing this code. We need to fix the<br>
+ // parsing here.<br></blockquote><div><br></div></div></div><div>Any progress on this? This regressed working functionality.</div></div></div></div></blockquote><div><br></div><div style>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?)</div>
<div style><br></div><div style>Thx,</div><div style>/Manuel</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ verifyFormat("@{ @\"one\" : @\n"<br>
+ "{ @2 : @1 }\n"<br>
+ "}");<br>
+ verifyFormat("@{ @\"one\" : @\n"<br>
+ "{ @2 : @1 }\n"<br>
+ ",\n"<br>
+ "}");<br>
+<br>
verifyFormat("@{ 1 > 2 ? @\"one\" : @\"two\" : 1 > 2 ? @1 : @2 }");<br>
verifyFormat("[self setDict:@{}");<br>
verifyFormat("[self setDict:@{ @1 : @2 }");<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div></div><br></div></div>
</blockquote></div><br></div></div>