This is an archive of the discontinued LLVM Phabricator instance.

Differential D85590

[clang][HeaderInsert] Do not treat defines with values as header guards
ClosedPublic

Authored by kadircet on Aug 8 2020, 4:03 PM.

Details

Reviewers
hokein
Commits
rGea8e71c3da56: [clang][HeaderInsert] Do not treat defines with values as header guards
Summary

This was resulting in inserting headers at bogus locations, see
https://github.com/ycm-core/YouCompleteMe/issues/3736 for an example.

Diff Detail

Event Timeline

kadircet created this revision.Aug 8 2020, 4:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2020, 4:03 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
kadircet requested review of this revision.Aug 8 2020, 4:03 PM
Harbormaster completed remote builds in B67603: Diff 284152.Aug 8 2020, 4:32 PM
hokein accepted this revision.Aug 9 2020, 11:21 PM
hokein added inline comments.
clang/unittests/Tooling/HeaderIncludesTest.cpp
544

could you move this to the HeaderGuard section of this file (~Line 349)?

This revision is now accepted and ready to land.Aug 9 2020, 11:21 PM
kadircet updated this revision to Diff 284702.Aug 11 2020, 7:02 AM
  • Move new test closer to other FakeHeaderGuard tests.
This revision was landed with ongoing or failed builds.Aug 11 2020, 7:09 AM
Closed by commit rGea8e71c3da56: [clang][HeaderInsert] Do not treat defines with values as header guards (authored by kadircet). · Explain Why
This revision was automatically updated to reflect the committed changes.
kadircet added a commit: rGea8e71c3da56: [clang][HeaderInsert] Do not treat defines with values as header guards.
Harbormaster completed remote builds in B67890: Diff 284702.Aug 11 2020, 7:51 AM

Revision Contents

PathSize
clang/
lib/
Tooling/
Inclusions/
3 lines
unittests/
Tooling/
12 lines

Diff 284704

clang/lib/Tooling/Inclusions/HeaderIncludes.cpp

Show First 20 Lines • Show All 94 Lines • ▼ Show 20 Lines auto ConsumeHeaderGuardAndComment =
}); });
}; };
return std::max( return std::max(
// #ifndef/#define // #ifndef/#define
ConsumeHeaderGuardAndComment( ConsumeHeaderGuardAndComment(
[](const SourceManager &SM, Lexer &Lex, Token Tok) -> unsigned { [](const SourceManager &SM, Lexer &Lex, Token Tok) -> unsigned {
if (checkAndConsumeDirectiveWithName(Lex, "ifndef", Tok)) { if (checkAndConsumeDirectiveWithName(Lex, "ifndef", Tok)) {
skipComments(Lex, Tok); skipComments(Lex, Tok);
if (checkAndConsumeDirectiveWithName(Lex, "define", Tok)) if (checkAndConsumeDirectiveWithName(Lex, "define", Tok) &&
Tok.isAtStartOfLine())
return SM.getFileOffset(Tok.getLocation()); return SM.getFileOffset(Tok.getLocation());
} }
return 0; return 0;
}), }),
// #pragma once // #pragma once
ConsumeHeaderGuardAndComment( ConsumeHeaderGuardAndComment(
[](const SourceManager &SM, Lexer &Lex, Token Tok) -> unsigned { [](const SourceManager &SM, Lexer &Lex, Token Tok) -> unsigned {
if (checkAndConsumeDirectiveWithName(Lex, "pragma", Tok, if (checkAndConsumeDirectiveWithName(Lex, "pragma", Tok,
▲ Show 20 Lines • Show All 263 Lines • Show Last 20 Lines

clang/unittests/Tooling/HeaderIncludesTest.cpp

Show First 20 Lines • Show All 341 Lines • ▼ Show 20 Lines std::string Code = "// comment \n"
"#define 1\n"; "#define 1\n";
std::string Expected = "// comment \n" std::string Expected = "// comment \n"
"#include <vector>\n" "#include <vector>\n"
"#ifndef X\n" "#ifndef X\n"
"#define 1\n"; "#define 1\n";
EXPECT_EQ(Expected, insert(Code, "<vector>")); EXPECT_EQ(Expected, insert(Code, "<vector>"));
} }
TEST_F(HeaderIncludesTest, FakeHeaderGuardIfnDef) {
std::string Code = "#ifndef A_H\n"
"#define A_H 1\n"
"#endif";
std::string Expected = "#include \"b.h\"\n"
"#ifndef A_H\n"
"#define A_H 1\n"
"#endif";
EXPECT_EQ(Expected, insert(Code, "\"b.h\""));
}
TEST_F(HeaderIncludesTest, HeaderGuardWithComment) { TEST_F(HeaderIncludesTest, HeaderGuardWithComment) {
std::string Code = "// comment \n" std::string Code = "// comment \n"
"#ifndef X // comment\n" "#ifndef X // comment\n"
"// comment\n" "// comment\n"
"/* comment\n" "/* comment\n"
"*/\n" "*/\n"
"/* comment */ #define X\n" "/* comment */ #define X\n"
"int x;\n" "int x;\n"
▲ Show 20 Lines • Show All 166 Lines • ▼ Show 20 LinesTEST_F(HeaderIncludesTest, CanDeleteAfterCode) {
std::string Code = "#include \"a.h\"\n" std::string Code = "#include \"a.h\"\n"
"void f() {}\n" "void f() {}\n"
"#include \"b.h\"\n"; "#include \"b.h\"\n";
std::string Expected = "#include \"a.h\"\n" std::string Expected = "#include \"a.h\"\n"
"void f() {}\n"; "void f() {}\n";
EXPECT_EQ(Expected, remove(Code, "\"b.h\"")); EXPECT_EQ(Expected, remove(Code, "\"b.h\""));
} }
} // namespace } // namespace
hokeinUnsubmitted
Not Done
ReplyInline Actions

could you move this to the HeaderGuard section of this file (~Line 349)?

hokein: could you move this to the `HeaderGuard section` of this file (~Line 349)?
} // namespace tooling } // namespace tooling
} // namespace clang } // namespace clang