<div style="font-family: arial, helvetica, sans-serif; font-size: 10pt"><div dir="ltr"><div class="gmail_default" style><br></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Jan 2, 2013 at 5:30 PM, Manuel Klimek <span dir="ltr"><<a href="mailto:klimek@google.com" target="_blank" class="cremed">klimek@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: klimek<br>
Date: Wed Jan 2 10:30:12 2013<br>
New Revision: 171393<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=171393&view=rev" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project?rev=171393&view=rev</a><br>
Log:<br>
Fixes use of unescaped newlines when formatting preprocessor directives.<br>
<br>
This is the first step towards handling preprocessor directives. This<br>
patch only fixes the most pressing issue, namely correctly escaping<br>
newlines for tokens within a sequence of a preprocessor directive.<br>
<br>
The next step will be to fix incorrect format decisions on #define<br>
directives.<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=171393&r1=171392&r2=171393&view=diff" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/Format.cpp?rev=171393&r1=171392&r2=171393&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Format/Format.cpp (original)<br>
+++ cfe/trunk/lib/Format/Format.cpp Wed Jan 2 10:30:12 2013<br>
@@ -434,9 +434,21 @@<br>
/// each \c FormatToken.<br>
void replaceWhitespace(const FormatToken &Tok, unsigned NewLines,<br>
unsigned Spaces) {<br>
+ std::string NewLineText;<br>
+ if (!Line.InPPDirective) {<br>
+ NewLineText = std::string(NewLines, '\n');<br>
+ } else if (NewLines > 0) {<br>
+ unsigned Offset =<br>
+ SourceMgr.getSpellingColumnNumber(Tok.WhiteSpaceStart) - 1;<br></blockquote><div><br></div><div style>This is probably wrong as we might have already changed the position of the previous token. Instead, you need to pass in the column number of the previous token. This is called from addTokenToState(). I would store the previous column number (State.Column) there (immediately after "if (Newline)") and pass it to replaceWhitespace().</div>
<div style> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ for (unsigned i = 0; i < NewLines; ++i) {<br>
+ NewLineText += std::string(Style.ColumnLimit - Offset - 1, ' ');<br>
+ NewLineText += "\\\n";<br>
+ Offset = 0;<br>
+ }<br>
+ }<br>
Replaces.insert(tooling::Replacement(<br>
SourceMgr, Tok.WhiteSpaceStart, Tok.WhiteSpaceLength,<br>
- std::string(NewLines, '\n') + std::string(Spaces, ' ')));<br>
+ NewLineText + std::string(Spaces, ' ')));<br>
}<br>
<br>
/// \brief Add a new line and the required indent before the first Token<br>
@@ -634,6 +646,9 @@<br>
next();<br>
if (Index >= Tokens.size())<br>
return;<br>
+ // It is the responsibility of the UnwrappedLineParser to make sure<br>
+ // this sequence is not produced inside an unwrapped line.<br>
+ assert(Tokens[Index].Tok.getIdentifierInfo() != NULL);<br>
switch (Tokens[Index].Tok.getIdentifierInfo()->getPPKeywordID()) {<br>
case tok::pp_include:<br>
case tok::pp_import:<br>
@@ -969,7 +984,10 @@<br>
<br>
// Consume and record whitespace until we find a significant token.<br>
while (<a href="http://FormatTok.Tok.is" target="_blank" class="cremed">FormatTok.Tok.is</a>(tok::unknown)) {<br>
- FormatTok.NewlinesBefore += tokenText(FormatTok.Tok).count('\n');<br>
+ StringRef Text = tokenText(FormatTok.Tok);<br>
+ FormatTok.NewlinesBefore += Text.count('\n');<br>
+ FormatTok.HasUnescapedNewline =<br>
+ Text.count("\\\n") != FormatTok.NewlinesBefore;<br>
FormatTok.WhiteSpaceLength += FormatTok.Tok.getLength();<br>
<br>
if (<a href="http://FormatTok.Tok.is" target="_blank" class="cremed">FormatTok.Tok.is</a>(tok::eof))<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=171393&r1=171392&r2=171393&view=diff" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/UnwrappedLineParser.cpp?rev=171393&r1=171392&r2=171393&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Format/UnwrappedLineParser.cpp (original)<br>
+++ cfe/trunk/lib/Format/UnwrappedLineParser.cpp Wed Jan 2 10:30:12 2013<br>
@@ -80,13 +80,25 @@<br>
}<br>
<br>
void UnwrappedLineParser::parsePPDirective() {<br>
- while (!eof()) {<br>
- nextToken();<br>
- if (FormatTok.NewlinesBefore > 0) {<br>
- addUnwrappedLine();<br>
- return;<br>
- }<br>
+ assert(<a href="http://FormatTok.Tok.is" target="_blank" class="cremed">FormatTok.Tok.is</a>(tok::hash) && "'#' expected");<br>
+ nextToken();<br>
+<br>
+ Line.InPPDirective = true;<br>
+ if (FormatTok.Tok.getIdentifierInfo() == NULL) {<br>
+ addUnwrappedLine();<br>
+ Line.InPPDirective = false;<br>
+ return;<br>
}<br>
+<br>
+ do {<br>
+ if (FormatTok.NewlinesBefore > 0 &&<br>
+ FormatTok.HasUnescapedNewline) {<br>
+ break;<br>
+ }<br>
+ nextToken();<br>
+ } while (!eof());<br>
+ addUnwrappedLine();<br>
+ Line.InPPDirective = false;<br>
}<br>
<br>
void UnwrappedLineParser::parseComments() {<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=171393&r1=171392&r2=171393&view=diff" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/UnwrappedLineParser.h?rev=171393&r1=171392&r2=171393&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Format/UnwrappedLineParser.h (original)<br>
+++ cfe/trunk/lib/Format/UnwrappedLineParser.h Wed Jan 2 10:30:12 2013<br>
@@ -30,7 +30,8 @@<br>
/// \brief A wrapper around a \c Token storing information about the<br>
/// whitespace characters preceeding it.<br>
struct FormatToken {<br>
- FormatToken() : NewlinesBefore(0), WhiteSpaceLength(0) {<br>
+ FormatToken()<br>
+ : NewlinesBefore(0), HasUnescapedNewline(false), WhiteSpaceLength(0) {<br>
}<br>
<br>
/// \brief The \c Token.<br>
@@ -42,6 +43,10 @@<br>
/// and thereby e.g. leave an empty line between two function definitions.<br>
unsigned NewlinesBefore;<br>
<br>
+ /// \brief Whether there is at least one unescaped newline before the \c<br>
+ /// Token.<br>
+ bool HasUnescapedNewline;<br>
+<br>
/// \brief The location of the start of the whitespace immediately preceeding<br>
/// the \c Token.<br>
///<br>
@@ -60,7 +65,7 @@<br>
/// \c UnwrappedLineFormatter. The key property is that changing the formatting<br>
/// within an unwrapped line does not affect any other unwrapped lines.<br>
struct UnwrappedLine {<br>
- UnwrappedLine() : Level(0) {<br>
+ UnwrappedLine() : Level(0), InPPDirective(false) {<br>
}<br>
<br>
/// \brief The \c Token comprising this \c UnwrappedLine.<br>
@@ -68,6 +73,9 @@<br>
<br>
/// \brief The indent level of the \c UnwrappedLine.<br>
unsigned Level;<br>
+<br>
+ /// \brief Whether this \c UnwrappedLine is part of a preprocessor directive.<br>
+ bool InPPDirective;<br>
};<br>
<br>
class UnwrappedLineConsumer {<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=171393&r1=171392&r2=171393&view=diff" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=171393&r1=171392&r2=171393&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/unittests/Format/FormatTest.cpp (original)<br>
+++ cfe/trunk/unittests/Format/FormatTest.cpp Wed Jan 2 10:30:12 2013<br>
@@ -373,6 +373,37 @@<br>
" \"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\" };");<br>
}<br>
<br>
+TEST_F(FormatTest, FormatsSmallMacroDefinitionsInSingleLine) {<br>
+ verifyFormat("#define ALooooooooooooooooooooooooooooooooooooooongMacro("<br>
+ " \\\n"<br>
+ " aLoooooooooooooooooooooooongFuuuuuuuuuuuuuunctiooooooooo)");<br>
+}<br>
+<br>
+TEST_F(FormatTest, BreaksOnHashWhenDirectiveIsInvalid) {<br>
+ EXPECT_EQ("#\n;", format("#;"));<br>
+}<br>
+<br>
+TEST_F(FormatTest, UnescapedEndOfLineEndsPPDirective) {<br>
+ EXPECT_EQ("#line 42 \"test\"\n",<br>
+ format("# \\\n line \\\n 42 \\\n \"test\"\n"));<br>
+ EXPECT_EQ("#define A B\n", format("# \\\n define \\\n A \\\n B\n"));<br>
+}<br>
+<br>
+TEST_F(FormatTest, EndOfFileEndsPPDirective) {<br>
+ EXPECT_EQ("#line 42 \"test\"",<br>
+ format("# \\\n line \\\n 42 \\\n \"test\""));<br>
+ EXPECT_EQ("#define A B", format("# \\\n define \\\n A \\\n B"));<br>
+}<br>
+<br>
+TEST_F(FormatTest, MixingPreprocessorDirectivesAndNormalCode) {<br>
+ verifyFormat("#define ALooooooooooooooooooooooooooooooooooooooongMacro("<br>
+ " \\\n"<br>
+ " aLoooooooooooooooooooooooongFuuuuuuuuuuuuuunctiooooooooo)\n"<br>
+ "\n"<br>
+ "AlooooooooooooooooooooooooooooooooooooooongCaaaaaaaaaal(\n"<br>
+ " aLooooooooooooooooooooooonPaaaaaaaaaaaaaaaaaaaaarmmmm);\n");<br>
+}<br>
+<br>
//===----------------------------------------------------------------------===//<br>
// Line break tests.<br>
//===----------------------------------------------------------------------===//<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" class="cremed">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank" class="cremed">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br></div></div></div>