r271883 - [clang-format] make header guard identification stricter (with Lexer).
Eric Liu via cfe-commits
cfe-commits at lists.llvm.org
Sat Jun 11 04:54:05 PDT 2016
Hi Mehdi,
No, it is not intended... thanks for pointing that out! I have fixed it at
rL272465.
Thanks,
Eric
On Fri, Jun 10, 2016 at 10:42 PM Mehdi Amini <mehdi.amini at apple.com> wrote:
> Hi Eric,
>
> > On Jun 6, 2016, at 4:00 AM, Eric Liu via cfe-commits <
> cfe-commits at 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 at lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160611/6fa0cf40/attachment-0001.html>
More information about the cfe-commits
mailing list