<div dir="ltr"><div class="gmail_default" style>On Mon, Jan 7, 2013 at 10:21 PM, Argyrios Kyrtzidis <span dir="ltr"><<a href="mailto:akyrtzi@gmail.com" target="_blank">akyrtzi@gmail.com</a>></span> wrote:<br></div><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 style="word-wrap:break-word"><div><div class="im"><div>On Jan 7, 2013, at 12:58 PM, Manuel Klimek <<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>> wrote:</div>
<br><blockquote type="cite"><div dir="ltr"><div>On Mon, Jan 7, 2013 at 9:27 PM, Argyrios Kyrtzidis <span dir="ltr"><<a href="mailto:akyrtzi@gmail.com" target="_blank">akyrtzi@gmail.com</a>></span> wrote:<br></div><div class="gmail_extra">
<div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word">
<div><div>On Jan 7, 2013, at 12:10 PM, Manuel Klimek <<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>> wrote:</div>
<br><blockquote type="cite"><div dir="ltr"><div>On Mon, Jan 7, 2013 at 8:53 PM, Argyrios Kyrtzidis <span dir="ltr"><<a href="mailto:akyrtzi@gmail.com" target="_blank">akyrtzi@gmail.com</a>></span> wrote:<br></div><div class="gmail_extra">
<div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div>On Jan 5, 2013, at 2:56 PM, Manuel Klimek <<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>> wrote:<br>
<br>
> Author: klimek<br>
> Date: Sat Jan 5 16:56:06 2013<br>
> New Revision: 171640<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=171640&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=171640&view=rev</a><br>
> Log:<br>
> Fixes parsing of hash tokens in the middle of a line.<br>
><br>
> To parse # correctly, we need to know whether it is the first token in a<br>
> line - we can deduct this either from the whitespace or seeing that the<br>
> token is the first in the file - we already calculate this information.<br>
> This patch moves the identification of the first token into the<br>
> getNextToken method and stores it inside the FormatToken, so the<br>
> UnwrappedLineParser can stay independent of the SourceManager.<br>
<br>
</div>This may be silly since I'm not familiar with libFormat, but shouldn't you just check clang::Token::isAtStartOfLine() ?<br></blockquote><div><br></div><div>After trying it out it looks like isAtStartOfLine tells me whether it's the first character (whitespace included).</div>
<div>But we need to know whether it's the first token in the line, disregarding possible whitespace...</div></div></div></div></blockquote><div><br></div></div><div>Your unit-test only includes a case where a hash has another token before it; it's not clear to me why, if isAtStartOfLine is true, it makes a difference if there is whitespace or not, for parsing hash tokens.</div>
</div></blockquote><div><br></div><div>Note that the added unit test doesn't add the full story - there were other unit tests breaking before I added the IsFirst check (also, other unit tests break if I use isAtStartOfLine).</div>
<div><br></div><div>The problem is that isStartOfLine is not true for the hash token in the case of</div><div> #define ...</div></div></div></div></blockquote><div><br></div></div><div>Ah, you are doing "Lex.SetKeepWhitespaceMode(true)", that's why isAtStartOfLine() is not doing what I suggested.</div>
<div class="im"><br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>We could probably also deduce it from isAtStartOfLine of the first (possibly whitespace) token we see before the hash... I can play around with that some more...</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div>In any case, "IsFirst", as you defined it, can be deducted from isAtStartOfLine and WhiteSpaceLength, right ?</div>
</div></blockquote><div><br></div><div>IsFirst actually is true if it's the first token in the *file*.</div></div></div></div></blockquote><div><br></div></div>Thanks, for clarifying.</div></div></blockquote><div><br>
</div><div style>So, I looked at it again; we could calculate IsFirstInLine for our FormatToken, but since we need the global IsFirst anyway in a different place, I think that wouldn't help much...</div><div style><br>
</div><div style>So unless you feel strongly about it, I'd prefer to leave it as is.</div><div style><br></div><div style>Cheers,</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 style="word-wrap:break-word"><div><div class="h5"><div><br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div>Cheers,</div><div>/Manuel</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div><span><font color="#888888"><div><br></div><div>-Argyrios</div></font></span><div><div><br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div><br></div><div>Or am I missing something?</div><div><br></div><div>Thanks!</div>
<div>/Manuel</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span><font color="#888888"><br>
-Argyrios<br>
</font></span><div><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=171640&r1=171639&r2=171640&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/Format.cpp?rev=171640&r1=171639&r2=171640&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/lib/Format/Format.cpp (original)<br>
> +++ cfe/trunk/lib/Format/Format.cpp Sat Jan 5 16:56:06 2013<br>
> @@ -483,8 +483,7 @@<br>
><br>
> unsigned Newlines =<br>
> std::min(Token.NewlinesBefore, Style.MaxEmptyLinesToKeep + 1);<br>
> - unsigned Offset = SourceMgr.getFileOffset(Token.WhiteSpaceStart);<br>
> - if (Newlines == 0 && Offset != 0)<br>
> + if (Newlines == 0 && !Token.IsFirst)<br>
> Newlines = 1;<br>
> unsigned Indent = Line.Level * 2;<br>
> if ((<a href="http://token.tok.is/" target="_blank">Token.Tok.is</a>(tok::kw_public) || <a href="http://token.tok.is/" target="_blank">Token.Tok.is</a>(tok::kw_protected) ||<br>
> @@ -685,9 +684,10 @@<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>
> + // Hashes in the middle of a line can lead to any strange token<br>
> + // sequence.<br>
> + if (Tokens[Index].Tok.getIdentifierInfo() == NULL)<br>
> + return;<br>
> switch (Tokens[Index].Tok.getIdentifierInfo()->getPPKeywordID()) {<br>
> case tok::pp_include:<br>
> case tok::pp_import:<br>
> @@ -1033,6 +1033,8 @@<br>
> Lex.LexFromRawLexer(FormatTok.Tok);<br>
> StringRef Text = tokenText(FormatTok.Tok);<br>
> FormatTok.WhiteSpaceStart = FormatTok.Tok.getLocation();<br>
> + if (SourceMgr.getFileOffset(FormatTok.WhiteSpaceStart) == 0)<br>
> + FormatTok.IsFirst = true;<br>
><br>
> // Consume and record whitespace until we find a significant token.<br>
> while (<a href="http://formattok.tok.is/" target="_blank">FormatTok.Tok.is</a>(tok::unknown)) {<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=171640&r1=171639&r2=171640&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/UnwrappedLineParser.cpp?rev=171640&r1=171639&r2=171640&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/lib/Format/UnwrappedLineParser.cpp (original)<br>
> +++ cfe/trunk/lib/Format/UnwrappedLineParser.cpp Sat Jan 5 16:56:06 2013<br>
> @@ -470,7 +470,9 @@<br>
><br>
> void UnwrappedLineParser::readToken() {<br>
> FormatTok = Tokens->getNextToken();<br>
> - while (!Line.InPPDirective && <a href="http://formattok.tok.is/" target="_blank">FormatTok.Tok.is</a>(tok::hash)) {<br>
> + while (!Line.InPPDirective && <a href="http://formattok.tok.is/" target="_blank">FormatTok.Tok.is</a>(tok::hash) &&<br>
> + ((FormatTok.NewlinesBefore > 0 && FormatTok.HasUnescapedNewline) ||<br>
> + FormatTok.IsFirst)) {<br>
> // FIXME: This is incorrect - the correct way is to create a<br>
> // data structure that will construct the parts around the preprocessor<br>
> // directive as a structured \c UnwrappedLine.<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=171640&r1=171639&r2=171640&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/UnwrappedLineParser.h?rev=171640&r1=171639&r2=171640&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/lib/Format/UnwrappedLineParser.h (original)<br>
> +++ cfe/trunk/lib/Format/UnwrappedLineParser.h Sat Jan 5 16:56:06 2013<br>
> @@ -31,7 +31,8 @@<br>
> /// whitespace characters preceeding it.<br>
> struct FormatToken {<br>
> FormatToken()<br>
> - : NewlinesBefore(0), HasUnescapedNewline(false), WhiteSpaceLength(0) {<br>
> + : NewlinesBefore(0), HasUnescapedNewline(false), WhiteSpaceLength(0),<br>
> + IsFirst(false) {<br>
> }<br>
><br>
> /// \brief The \c Token.<br>
> @@ -56,6 +57,9 @@<br>
> /// \brief The length in characters of the whitespace immediately preceeding<br>
> /// the \c Token.<br>
> unsigned WhiteSpaceLength;<br>
> +<br>
> + /// \brief Indicates that this is the first token.<br>
> + bool IsFirst;<br>
> };<br>
><br>
> /// \brief An unwrapped line is a sequence of \c Token, that we would like to<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=171640&r1=171639&r2=171640&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=171640&r1=171639&r2=171640&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/unittests/Format/FormatTest.cpp (original)<br>
> +++ cfe/trunk/unittests/Format/FormatTest.cpp Sat Jan 5 16:56:06 2013<br>
> @@ -470,6 +470,10 @@<br>
> EXPECT_EQ("{\n {\n#define A\n }\n}", format("{{\n#define A\n}}"));<br>
> }<br>
><br>
> +TEST_F(FormatTest, FormatHashIfNotAtStartOfLine) {<br>
> + verifyFormat("{\n {\n a #c;\n }\n}");<br>
> +}<br>
> +<br>
> // FIXME: write test for unbalanced braces in macros...<br>
> // FIXME: test # inside a normal statement (like {#define A b})<br>
><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>
<br>
</div></blockquote></div><br></div></div>
</blockquote></div></div></div><br></div></blockquote></div><br></div></div>
</blockquote></div><br></div></div></div></blockquote></div><br></div></div>