<div dir="ltr"><div>Hi Mehdi,</div><div><br></div><div>No, it is not intended... thanks for pointing that out! I have fixed it at <span style="color:rgb(33,33,33);font-family:"helvetica neue",helvetica,arial,sans-serif;line-height:1.5">rL272465.</span></div><div><span style="color:rgb(33,33,33);font-family:"helvetica neue",helvetica,arial,sans-serif;line-height:1.5"><br></span></div><div><span style="color:rgb(33,33,33);font-family:"helvetica neue",helvetica,arial,sans-serif;line-height:1.5">Thanks,</span></div><div><span style="color:rgb(33,33,33);font-family:"helvetica neue",helvetica,arial,sans-serif;line-height:1.5">Eric</span><br></div></div><br><div class="gmail_quote"><div dir="ltr">On Fri, Jun 10, 2016 at 10:42 PM Mehdi Amini <<a href="mailto:mehdi.amini@apple.com">mehdi.amini@apple.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Eric,<br>
<br>
> On Jun 6, 2016, at 4:00 AM, Eric Liu via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br>
><br>
> Author: ioeric<br>
> Date: Mon Jun 6 06:00:13 2016<br>
> New Revision: 271883<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=271883&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=271883&view=rev</a><br>
> Log:<br>
> [clang-format] make header guard identification stricter (with Lexer).<br>
><br>
> Summary: make header guard identification stricter with Lexer.<br>
><br>
> Reviewers: djasper<br>
><br>
> Subscribers: klimek, cfe-commits<br>
><br>
> Differential Revision: <a href="http://reviews.llvm.org/D20959" rel="noreferrer" target="_blank">http://reviews.llvm.org/D20959</a><br>
><br>
> Modified:<br>
> cfe/trunk/lib/Format/Format.cpp<br>
> cfe/trunk/unittests/Format/CleanupTest.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=271883&r1=271882&r2=271883&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/Format.cpp?rev=271883&r1=271882&r2=271883&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/lib/Format/Format.cpp (original)<br>
> +++ cfe/trunk/lib/Format/Format.cpp Mon Jun 6 06:00:13 2016<br>
> @@ -1436,6 +1436,49 @@ inline bool isHeaderInsertion(const tool<br>
> llvm::Regex(IncludeRegexPattern).match(Replace.getReplacementText());<br>
> }<br>
><br>
> +void skipComments(Lexer &Lex, Token &Tok) {<br>
> + while (Tok.is(tok::comment))<br>
> + if (Lex.LexFromRawLexer(Tok))<br>
> + return;<br>
> +}<br>
> +<br>
> +// Check if a sequence of tokens is like "#<Name> <raw_identifier>". If it is,<br>
> +// \p Tok will be the token after this directive; otherwise, it can be any token<br>
> +// after the given \p Tok (including \p Tok).<br>
> +bool checkAndConsumeDirectiveWithName(Lexer &Lex, StringRef Name, Token &Tok) {<br>
> + bool Matched = Tok.is(tok::hash) && !Lex.LexFromRawLexer(Tok) &&<br>
> + Tok.is(tok::raw_identifier) &&<br>
> + Tok.getRawIdentifier() == Name && !Lex.LexFromRawLexer(Tok) &&<br>
> + Tok.is(tok::raw_identifier);<br>
> + if (Matched)<br>
> + Lex.LexFromRawLexer(Tok);<br>
> + return Matched;<br>
> +}<br>
> +<br>
> +unsigned getOffsetAfterHeaderGuardsAndComments(StringRef FileName,<br>
> + StringRef Code,<br>
> + FormatStyle Style) {<br>
<br>
<br>
FormatStyle is 360B, did you really intended to pass it by value here?<br>
<br>
--<br>
Mehdi<br>
<br>
<br>
> + std::unique_ptr<Environment> Env =<br>
> + Environment::CreateVirtualEnvironment(Code, FileName, /*Ranges=*/{});<br>
> + const SourceManager &SourceMgr = Env->getSourceManager();<br>
> + Lexer Lex(Env->getFileID(), SourceMgr.getBuffer(Env->getFileID()), SourceMgr,<br>
> + getFormattingLangOpts(Style));<br>
> + Token Tok;<br>
> + // Get the first token.<br>
> + Lex.LexFromRawLexer(Tok);<br>
> + skipComments(Lex, Tok);<br>
> + unsigned AfterComments = SourceMgr.getFileOffset(Tok.getLocation());<br>
> + if (checkAndConsumeDirectiveWithName(Lex, "ifndef", Tok)) {<br>
> + skipComments(Lex, Tok);<br>
> + if (checkAndConsumeDirectiveWithName(Lex, "define", Tok))<br>
> + return SourceMgr.getFileOffset(Tok.getLocation());<br>
> + }<br>
> + return AfterComments;<br>
> +}<br>
> +<br>
> +// FIXME: we also need to insert a '\n' at the end of the code if we have an<br>
> +// insertion with offset Code.size(), and there is no '\n' at the end of the<br>
> +// code.<br>
> // FIXME: do not insert headers into conditional #include blocks, e.g. #includes<br>
> // surrounded by compile condition "#if...".<br>
> // FIXME: do not insert existing headers.<br>
> @@ -1469,20 +1512,6 @@ fixCppIncludeInsertions(StringRef Code,<br>
> StringRef FileName = Replaces.begin()->getFilePath();<br>
> IncludeCategoryManager Categories(Style, FileName);<br>
><br>
> - std::unique_ptr<Environment> Env =<br>
> - Environment::CreateVirtualEnvironment(Code, FileName, /*Ranges=*/{});<br>
> - const SourceManager &SourceMgr = Env->getSourceManager();<br>
> - Lexer Lex(Env->getFileID(), SourceMgr.getBuffer(Env->getFileID()), SourceMgr,<br>
> - getFormattingLangOpts(Style));<br>
> - Token Tok;<br>
> - // All new headers should be inserted after this offset.<br>
> - int MinInsertOffset = Code.size();<br>
> - while (!Lex.LexFromRawLexer(Tok)) {<br>
> - if (Tok.isNot(tok::comment)) {<br>
> - MinInsertOffset = SourceMgr.getFileOffset(Tok.getLocation());<br>
> - break;<br>
> - }<br>
> - }<br>
> // Record the offset of the end of the last include in each category.<br>
> std::map<int, int> CategoryEndOffsets;<br>
> // All possible priorities.<br>
> @@ -1491,26 +1520,25 @@ fixCppIncludeInsertions(StringRef Code,<br>
> for (const auto &Category : Style.IncludeCategories)<br>
> Priorities.insert(Category.Priority);<br>
> int FirstIncludeOffset = -1;<br>
> - bool HeaderGuardFound = false;<br>
> + // All new headers should be inserted after this offset.<br>
> + unsigned MinInsertOffset =<br>
> + getOffsetAfterHeaderGuardsAndComments(FileName, Code, Style);<br>
> StringRef TrimmedCode = Code.drop_front(MinInsertOffset);<br>
> SmallVector<StringRef, 32> Lines;<br>
> TrimmedCode.split(Lines, '\n');<br>
> - int Offset = MinInsertOffset;<br>
> + unsigned Offset = MinInsertOffset;<br>
> + unsigned NextLineOffset;<br>
> for (auto Line : Lines) {<br>
> + NextLineOffset = std::min(Code.size(), Offset + Line.size() + 1);<br>
> if (IncludeRegex.match(Line, &Matches)) {<br>
> StringRef IncludeName = Matches[2];<br>
> int Category = Categories.getIncludePriority(<br>
> IncludeName, /*CheckMainHeader=*/FirstIncludeOffset < 0);<br>
> - CategoryEndOffsets[Category] = Offset + Line.size() + 1;<br>
> + CategoryEndOffsets[Category] = NextLineOffset;<br>
> if (FirstIncludeOffset < 0)<br>
> FirstIncludeOffset = Offset;<br>
> }<br>
> - Offset += Line.size() + 1;<br>
> - // FIXME: make header guard matching stricter, e.g. consider #ifndef.<br>
> - if (!HeaderGuardFound && DefineRegex.match(Line)) {<br>
> - HeaderGuardFound = true;<br>
> - MinInsertOffset = Offset;<br>
> - }<br>
> + Offset = NextLineOffset;<br>
> }<br>
><br>
> // Populate CategoryEndOfssets:<br>
><br>
> Modified: cfe/trunk/unittests/Format/CleanupTest.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/CleanupTest.cpp?rev=271883&r1=271882&r2=271883&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/CleanupTest.cpp?rev=271883&r1=271882&r2=271883&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/unittests/Format/CleanupTest.cpp (original)<br>
> +++ cfe/trunk/unittests/Format/CleanupTest.cpp Mon Jun 6 06:00:13 2016<br>
> @@ -608,6 +608,86 @@ TEST_F(CleanUpReplacementsTest, CodeAfte<br>
> EXPECT_EQ(Expected, apply(Code, Replaces));<br>
> }<br>
><br>
> +TEST_F(CleanUpReplacementsTest, FakeHeaderGuardIfDef) {<br>
> + std::string Code = "// comment \n"<br>
> + "#ifdef X\n"<br>
> + "#define X\n";<br>
> + std::string Expected = "// comment \n"<br>
> + "#include <vector>\n"<br>
> + "#ifdef X\n"<br>
> + "#define X\n";<br>
> + tooling::Replacements Replaces = {createInsertion("#include <vector>")};<br>
> + EXPECT_EQ(Expected, apply(Code, Replaces));<br>
> +}<br>
> +<br>
> +TEST_F(CleanUpReplacementsTest, RealHeaderGuardAfterComments) {<br>
> + std::string Code = "// comment \n"<br>
> + "#ifndef X\n"<br>
> + "#define X\n"<br>
> + "int x;\n"<br>
> + "#define Y 1\n";<br>
> + std::string Expected = "// comment \n"<br>
> + "#ifndef X\n"<br>
> + "#define X\n"<br>
> + "#include <vector>\n"<br>
> + "int x;\n"<br>
> + "#define Y 1\n";<br>
> + tooling::Replacements Replaces = {createInsertion("#include <vector>")};<br>
> + EXPECT_EQ(Expected, apply(Code, Replaces));<br>
> +}<br>
> +<br>
> +TEST_F(CleanUpReplacementsTest, IfNDefWithNoDefine) {<br>
> + std::string Code = "// comment \n"<br>
> + "#ifndef X\n"<br>
> + "int x;\n"<br>
> + "#define Y 1\n";<br>
> + std::string Expected = "// comment \n"<br>
> + "#include <vector>\n"<br>
> + "#ifndef X\n"<br>
> + "int x;\n"<br>
> + "#define Y 1\n";<br>
> + tooling::Replacements Replaces = {createInsertion("#include <vector>")};<br>
> + EXPECT_EQ(Expected, apply(Code, Replaces));<br>
> +}<br>
> +<br>
> +TEST_F(CleanUpReplacementsTest, HeaderGuardWithComment) {<br>
> + std::string Code = "// comment \n"<br>
> + "#ifndef X // comment\n"<br>
> + "// comment\n"<br>
> + "/* comment\n"<br>
> + "*/\n"<br>
> + "/* comment */ #define X\n"<br>
> + "int x;\n"<br>
> + "#define Y 1\n";<br>
> + std::string Expected = "// comment \n"<br>
> + "#ifndef X // comment\n"<br>
> + "// comment\n"<br>
> + "/* comment\n"<br>
> + "*/\n"<br>
> + "/* comment */ #define X\n"<br>
> + "#include <vector>\n"<br>
> + "int x;\n"<br>
> + "#define Y 1\n";<br>
> + tooling::Replacements Replaces = {createInsertion("#include <vector>")};<br>
> + EXPECT_EQ(Expected, apply(Code, Replaces));<br>
> +}<br>
> +<br>
> +TEST_F(CleanUpReplacementsTest, EmptyCode) {<br>
> + std::string Code = "";<br>
> + std::string Expected = "#include <vector>\n";<br>
> + tooling::Replacements Replaces = {createInsertion("#include <vector>")};<br>
> + EXPECT_EQ(Expected, apply(Code, Replaces));<br>
> +}<br>
> +<br>
> +// FIXME: although this case does not crash, the insertion is wrong. A '\n'<br>
> +// should be inserted between the two #includes.<br>
> +TEST_F(CleanUpReplacementsTest, NoNewLineAtTheEndOfCode) {<br>
> + std::string Code = "#include <map>";<br>
> + std::string Expected = "#include <map>#include <vector>\n";<br>
> + tooling::Replacements Replaces = {createInsertion("#include <vector>")};<br>
> + EXPECT_EQ(Expected, apply(Code, Replaces));<br>
> +}<br>
> +<br>
> } // end namespace<br>
> } // end namespace format<br>
> } // end namespace clang<br>
><br>
><br>
> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
<br>
</blockquote></div>