sammccall created this revision.
sammccall added a reviewer: ilya-biryukov.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay,
ioeric.
This is needed to correctly handle checks that use IncludeInserter,
which is very common.
I couldn't find a totally safe example of a check to enable for testing,
I picked modernize-deprecated-headers which some will probably hate.
We should get configuration working...
This depends on https://reviews.llvm.org/D54691 which ensures our calls to
getFile(open=false)
don't break subsequent accesses via the FileManager.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D54694
Files:
clangd/ClangdUnit.cpp
clangd/Diagnostics.cpp
clangd/Headers.cpp
clangd/Headers.h
unittests/clangd/ClangdUnitTests.cpp
unittests/clangd/HeadersTests.cpp
Index: unittests/clangd/HeadersTests.cpp
===================================================================
--- unittests/clangd/HeadersTests.cpp
+++ unittests/clangd/HeadersTests.cpp
@@ -220,19 +220,20 @@
}
TEST_F(HeadersTest, DontInsertDuplicatePreferred) {
- std::vector<Inclusion> Inclusions = {
- {Range(), /*Written*/ "\"bar.h\"", /*Resolved*/ ""}};
- EXPECT_EQ(calculate(testPath("sub/bar.h"), "\"bar.h\"", Inclusions), "");
- EXPECT_EQ(calculate("\"x.h\"", "\"bar.h\"", Inclusions), "");
+ Inclusion Inc;
+ Inc.Written = "\"bar.h\"";
+ Inc.Resolved = "";
+ EXPECT_EQ(calculate(testPath("sub/bar.h"), "\"bar.h\"", {Inc}), "");
+ EXPECT_EQ(calculate("\"x.h\"", "\"bar.h\"", {Inc}), "");
}
TEST_F(HeadersTest, DontInsertDuplicateResolved) {
- std::string BarHeader = testPath("sub/bar.h");
- std::vector<Inclusion> Inclusions = {
- {Range(), /*Written*/ "fake-bar.h", /*Resolved*/ BarHeader}};
- EXPECT_EQ(calculate(BarHeader, "", Inclusions), "");
+ Inclusion Inc;
+ Inc.Written = "fake-bar.h";
+ Inc.Resolved = testPath("sub/bar.h");
+ EXPECT_EQ(calculate(Inc.Resolved, "", {Inc}), "");
// Do not insert preferred.
- EXPECT_EQ(calculate(BarHeader, "\"BAR.h\"", Inclusions), "");
+ EXPECT_EQ(calculate(Inc.Resolved, "\"BAR.h\"", {Inc}), "");
}
TEST_F(HeadersTest, PreferInserted) {
Index: unittests/clangd/ClangdUnitTests.cpp
===================================================================
--- unittests/clangd/ClangdUnitTests.cpp
+++ unittests/clangd/ClangdUnitTests.cpp
@@ -131,19 +131,28 @@
TEST(DiagnosticsTest, ClangTidy) {
Annotations Test(R"cpp(
+ #include $deprecated[["assert.h"]]
+
#define $macrodef[[SQUARE]](X) (X)*(X)
int main() {
- return [[sizeof]](sizeof(int));
+ return $doubled[[sizeof]](sizeof(int));
int y = 4;
return SQUARE($macroarg[[++]]y);
}
)cpp");
auto TU = TestTU::withCode(Test.code());
+ TU.HeaderFilename = "assert.h"; // Suppress "not found" error.
EXPECT_THAT(
TU.build().getDiagnostics(),
UnorderedElementsAre(
- Diag(Test.range(), "suspicious usage of 'sizeof(sizeof(...))' "
- "[bugprone-sizeof-expression]"),
+ AllOf(Diag(Test.range("deprecated"),
+ "inclusion of deprecated C++ header 'assert.h'; consider "
+ "using 'cassert' instead [modernize-deprecated-headers]"),
+ WithFix(Fix(Test.range("deprecated"), "<cassert>",
+ "change '\"assert.h\"' to '<cassert>'"))),
+ Diag(Test.range("doubled"),
+ "suspicious usage of 'sizeof(sizeof(...))' "
+ "[bugprone-sizeof-expression]"),
AllOf(
Diag(Test.range("macroarg"),
"side effects in the 1st macro argument 'X' are repeated in "
Index: clangd/Headers.h
===================================================================
--- clangd/Headers.h
+++ clangd/Headers.h
@@ -43,6 +43,8 @@
Range R; // Inclusion range.
std::string Written; // Inclusion name as written e.g. <vector>.
Path Resolved; // Resolved path of included file. Empty if not resolved.
+ unsigned HashOffset = 0; // Byte offset from start of file to #.
+ SrcMgr::CharacteristicKind FileKind = SrcMgr::C_User;
};
// Information captured about the inclusion graph in a translation unit.
Index: clangd/Headers.cpp
===================================================================
--- clangd/Headers.cpp
+++ clangd/Headers.cpp
@@ -34,13 +34,17 @@
CharSourceRange FilenameRange, const FileEntry *File,
StringRef /*SearchPath*/, StringRef /*RelativePath*/,
const Module * /*Imported*/,
- SrcMgr::CharacteristicKind /*FileType*/) override {
- if (SM.isInMainFile(HashLoc))
- Out->MainFileIncludes.push_back({
- halfOpenToRange(SM, FilenameRange),
- (IsAngled ? "<" + FileName + ">" : "\"" + FileName + "\"").str(),
- File ? File->tryGetRealPathName() : "",
- });
+ SrcMgr::CharacteristicKind FileKind) override {
+ if (SM.isWrittenInMainFile(HashLoc)) {
+ Out->MainFileIncludes.emplace_back();
+ auto &Inc = Out->MainFileIncludes.back();
+ Inc.R = halfOpenToRange(SM, FilenameRange);
+ Inc.Written =
+ (IsAngled ? "<" + FileName + ">" : "\"" + FileName + "\"").str();
+ Inc.Resolved = File ? File->tryGetRealPathName() : "";
+ Inc.HashOffset = SM.getFileOffset(HashLoc);
+ Inc.FileKind = FileKind;
+ }
if (File) {
auto *IncludingFileEntry = SM.getFileEntryForID(SM.getFileID(HashLoc));
if (!IncludingFileEntry) {
Index: clangd/Diagnostics.cpp
===================================================================
--- clangd/Diagnostics.cpp
+++ clangd/Diagnostics.cpp
@@ -190,8 +190,11 @@
} // namespace
raw_ostream &operator<<(raw_ostream &OS, const DiagBase &D) {
+ OS << "[";
if (!D.InsideMainFile)
- OS << "[in " << D.File << "] ";
+ OS << D.File << ":";
+ OS << D.Range.start << "-" << D.Range.end << "] ";
+
return OS << D.Message;
}
Index: clangd/ClangdUnit.cpp
===================================================================
--- clangd/ClangdUnit.cpp
+++ clangd/ClangdUnit.cpp
@@ -119,6 +119,88 @@
SourceManager *SourceMgr = nullptr;
};
+// When using a preamble, only preprocessor events outside its bounds are seen.
+// This is almost what we want: replaying transitive preprocessing wastes time.
+// However this confuses clang-tidy checks: they don't see any #includes!
+// So we replay the *non-transitive* #includes that appear in the main-file.
+// It would be nice to replay other events (macro definitions, ifdefs etc) but
+// this addresses the most common cases fairly cheaply.
+class ReplayPreamble : public PPCallbacks {
+ const IncludeStructure &Includes;
+ PPCallbacks *Delegate;
+ const SourceManager &SM;
+ Preprocessor &PP;
+ const LangOptions &LangOpts;
+
+ void replay() {
+ for (const auto &Inc : Includes.MainFileIncludes) {
+ const FileEntry *File = nullptr;
+ if (Inc.Resolved != "")
+ File = SM.getFileManager().getFile(Inc.Resolved);
+
+ StringRef WrittenFilename =
+ StringRef(Inc.Written).drop_front().drop_back();
+ bool Angled = StringRef(Inc.Written).startswith("<");
+
+ // Re-lex the #include directive to find its interesting parts.
+ StringRef Src = SM.getBufferData(SM.getMainFileID());
+ Lexer RawLexer(SM.getLocForStartOfFile(SM.getMainFileID()), LangOpts,
+ Src.begin(), Src.begin() + Inc.HashOffset, Src.end());
+ Token HashTok, IncludeTok, FilenameTok;
+ RawLexer.LexFromRawLexer(HashTok);
+ assert(HashTok.getKind() == tok::hash);
+ RawLexer.setParsingPreprocessorDirective(true);
+ RawLexer.LexFromRawLexer(IncludeTok);
+ IdentifierInfo *II = PP.getIdentifierInfo(IncludeTok.getRawIdentifier());
+ IncludeTok.setIdentifierInfo(II);
+ IncludeTok.setKind(II->getTokenID());
+ RawLexer.LexIncludeFilename(FilenameTok);
+
+ PP.getPPCallbacks()->InclusionDirective(
+ HashTok.getLocation(), IncludeTok, WrittenFilename, Angled,
+ CharSourceRange::getCharRange(FilenameTok.getLocation(),
+ FilenameTok.getEndLoc()),
+ File, "SearchPath", "RelPath", /*Imported=*/nullptr, Inc.FileKind);
+ if (File)
+ PP.getPPCallbacks()->FileSkipped(*File, FilenameTok, Inc.FileKind);
+ else {
+ SmallString<1> UnusedRecovery;
+ Delegate->FileNotFound(WrittenFilename, UnusedRecovery);
+ }
+ }
+ }
+
+public:
+ ReplayPreamble(const IncludeStructure &Includes, PPCallbacks *Delegate,
+ const SourceManager &SM, Preprocessor &PP,
+ const LangOptions &LangOpts)
+ : Includes(Includes), Delegate(Delegate), SM(SM), PP(PP),
+ LangOpts(LangOpts) {}
+
+ // In a normal compile, the preamble traverses the following structure:
+ //
+ // mainfile.cpp
+ // <built-in>
+ // ... macro definitions like __cplusplus ...
+ // <command-line>
+ // ... macro definitions for args like -Dfoo=bar ...
+ // "header1.h"
+ // ... header file contents ...
+ // "header2.h"
+ // ... header file contents ...
+ // ... main file contents ...
+ //
+ // When using a preamble, the "header1" and "header2" subtrees get skipped.
+ // We insert them right after the built-in header, which still appears.
+ void FileChanged(SourceLocation Loc, FileChangeReason Reason,
+ SrcMgr::CharacteristicKind Kind, FileID PrevFID) override {
+ // It'd be nice if there was a better way to identify built-in headers...
+ if (Reason == FileChangeReason::ExitFile &&
+ SM.getBuffer(PrevFID)->getBufferIdentifier() == "<built-in>")
+ replay();
+ }
+};
+
} // namespace
void dumpAST(ParsedAST &AST, raw_ostream &OS) {
@@ -171,8 +253,9 @@
// FIXME: this needs to be configurable, and we need to support .clang-tidy
// files and other options providers.
// These checks exercise the matcher- and preprocessor-based hooks.
- CTOpts.Checks =
- "bugprone-sizeof-expression,bugprone-macro-repeated-side-effects";
+ CTOpts.Checks = "bugprone-sizeof-expression,"
+ "bugprone-macro-repeated-side-effects,"
+ "modernize-deprecated-headers";
CTContext.emplace(llvm::make_unique<tidy::DefaultOptionsProvider>(
tidy::ClangTidyGlobalOptions(), CTOpts));
CTContext->setDiagnosticsEngine(&Clang->getDiagnostics());
@@ -190,6 +273,15 @@
// Copy over the includes from the preamble, then combine with the
// non-preamble includes below.
auto Includes = Preamble ? Preamble->Includes : IncludeStructure{};
+ // Replay the preamble includes so that clang-tidy checks can see them.
+ if (Preamble)
+ Clang->getPreprocessor().addPPCallbacks(llvm::make_unique<ReplayPreamble>(
+ Preamble->Includes, Clang->getPreprocessor().getPPCallbacks(),
+ Clang->getSourceManager(), Clang->getPreprocessor(),
+ Clang->getLangOpts()));
+ // Important: collectIncludeStructure is registered *after* ReplayPreamble!
+ // Otherwise we would collect the replayed includes again...
+ // (We can't *just* use the replayed includes, they don't have Resolved path).
Clang->getPreprocessor().addPPCallbacks(
collectIncludeStructureCallback(Clang->getSourceManager(), &Includes));
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
