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