kbobyrev updated this revision to Diff 429637.
kbobyrev marked 4 inline comments as done.
kbobyrev added a comment.
Address review comments.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D125468/new/
https://reviews.llvm.org/D125468
Files:
clang-tools-extra/clangd/Headers.cpp
clang-tools-extra/clangd/Headers.h
clang-tools-extra/clangd/IncludeCleaner.cpp
clang-tools-extra/clangd/unittests/HeadersTests.cpp
clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
Index: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
+++ clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
@@ -544,8 +544,7 @@
EXPECT_TRUE(
ReferencedFiles.User.contains(AST.getSourceManager().getMainFileID()));
EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty()));
- auto Unused = computeUnusedIncludes(AST);
- EXPECT_THAT(Unused, IsEmpty());
+ EXPECT_THAT(computeUnusedIncludes(AST), IsEmpty());
}
TEST(IncludeCleaner, RecursiveInclusion) {
@@ -576,8 +575,34 @@
findReferencedLocations(AST), AST.getIncludeStructure(),
AST.getCanonicalIncludes(), AST.getSourceManager());
EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty()));
- auto Unused = computeUnusedIncludes(AST);
- EXPECT_THAT(Unused, IsEmpty());
+ EXPECT_THAT(computeUnusedIncludes(AST), IsEmpty());
+}
+
+TEST(IncludeCleaner, IWYUPragmaExport) {
+ TestTU TU;
+ TU.Code = R"cpp(
+ #include "foo.h"
+ )cpp";
+ TU.AdditionalFiles["foo.h"] = R"cpp(
+ #ifndef FOO_H
+ #define FOO_H
+
+ #include "bar.h" // IWYU pragma: export
+
+ #endif
+ )cpp";
+ TU.AdditionalFiles["bar.h"] = guard(R"cpp(
+ void bar() {}
+ )cpp");
+ ParsedAST AST = TU.build();
+
+ auto ReferencedFiles = findReferencedFiles(
+ findReferencedLocations(AST), AST.getIncludeStructure(),
+ AST.getCanonicalIncludes(), AST.getSourceManager());
+ EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty()));
+ // FIXME: This is not correct: foo.h is unused but is not diagnosed as such
+ // because we ignore headers with IWYU export pragmas for now.
+ EXPECT_THAT(computeUnusedIncludes(AST), IsEmpty());
}
} // namespace
Index: clang-tools-extra/clangd/unittests/HeadersTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/HeadersTests.cpp
+++ clang-tools-extra/clangd/unittests/HeadersTests.cpp
@@ -406,7 +406,7 @@
#define RECURSIVE_H
#include "recursive.h"
-
+
#endif // RECURSIVE_H
)cpp";
@@ -418,6 +418,33 @@
EXPECT_FALSE(Includes.isSelfContained(getID("pp_depend.h", Includes)));
}
+TEST_F(HeadersTest, HasIWYUPragmas) {
+ FS.Files[MainFile] = R"cpp(
+#include "export.h"
+#include "begin_exports.h"
+#include "none.h"
+)cpp";
+ FS.Files["export.h"] = R"cpp(
+#pragma once
+#include "none.h" // IWYU pragma: export
+)cpp";
+ FS.Files["begin_exports.h"] = R"cpp(
+#pragma once
+// IWYU pragma: begin_exports
+#include "none.h"
+// IWYU pragma: end_exports
+)cpp";
+ FS.Files["none.h"] = R"cpp(
+#pragma once
+// Not a pragma.
+)cpp";
+
+ auto Includes = collectIncludes();
+ EXPECT_TRUE(Includes.hasIWYUExport(getID("export.h", Includes)));
+ EXPECT_TRUE(Includes.hasIWYUExport(getID("begin_exports.h", Includes)));
+ EXPECT_FALSE(Includes.hasIWYUExport(getID("none.h", Includes)));
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===================================================================
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -268,13 +268,17 @@
return true;
return false;
}
- // Headers without include guards have side effects and are not
- // self-contained, skip them.
assert(Inc.HeaderID);
+ auto HID = static_cast<IncludeStructure::HeaderID>(*Inc.HeaderID);
+ // FIXME: Ignore the headers with IWYU export pragmas for now, remove this
+ // check when we have more precise tracking of exported headers.
+ if (AST.getIncludeStructure().hasIWYUExport(HID))
+ return false;
auto FE = AST.getSourceManager().getFileManager().getFileRef(
- AST.getIncludeStructure().getRealPath(
- static_cast<IncludeStructure::HeaderID>(*Inc.HeaderID)));
+ AST.getIncludeStructure().getRealPath(HID));
assert(FE);
+ // Headers without include guards have side effects and are not
+ // self-contained, skip them.
if (!AST.getPreprocessor().getHeaderSearchInfo().isFileMultipleIncludeGuarded(
&FE->getFileEntry())) {
dlog("{0} doesn't have header guard and will not be considered unused",
Index: clang-tools-extra/clangd/Headers.h
===================================================================
--- clang-tools-extra/clangd/Headers.h
+++ clang-tools-extra/clangd/Headers.h
@@ -145,6 +145,10 @@
return !NonSelfContained.contains(ID);
}
+ bool hasIWYUExport(HeaderID ID) const {
+ return HasIWYUPragmas.contains(ID);
+ }
+
// Return all transitively reachable files.
llvm::ArrayRef<std::string> allHeaders() const { return RealPathNames; }
@@ -185,6 +189,9 @@
// Contains HeaderIDs of all non self-contained entries in the
// IncludeStructure.
llvm::DenseSet<HeaderID> NonSelfContained;
+ // Contains a set of headers that have either "IWYU pragma: export" or "IWYU
+ // pragma: begin_exports".
+ llvm::DenseSet<HeaderID> HasIWYUPragmas;
};
// Calculates insertion edit for including a new header in a file.
Index: clang-tools-extra/clangd/Headers.cpp
===================================================================
--- clang-tools-extra/clangd/Headers.cpp
+++ clang-tools-extra/clangd/Headers.cpp
@@ -24,6 +24,7 @@
const char IWYUPragmaKeep[] = "// IWYU pragma: keep";
const char IWYUPragmaExport[] = "// IWYU pragma: export";
+const char IWYUPragmaBeginExports[] = "// IWYU pragma: begin_exports";
class IncludeStructure::RecordHeaders : public PPCallbacks,
public CommentHandler {
@@ -127,31 +128,45 @@
}
}
- // Given:
- //
- // #include "foo.h"
- // #include "bar.h" // IWYU pragma: keep
- //
- // The order in which the callbacks will be triggered:
- //
- // 1. InclusionDirective("foo.h")
- // 2. HandleComment("// IWYU pragma: keep")
- // 3. InclusionDirective("bar.h")
- //
- // HandleComment will store the last location of "IWYU pragma: keep" (or
- // export) comment in the main file, so that when InclusionDirective is
- // called, it will know that the next inclusion is behind the IWYU pragma.
bool HandleComment(Preprocessor &PP, SourceRange Range) override {
- if (!inMainFile() || Range.getBegin().isMacroID())
- return false;
bool Err = false;
llvm::StringRef Text = SM.getCharacterData(Range.getBegin(), &Err);
- if (Err && !Text.consume_front(IWYUPragmaKeep) &&
- !Text.consume_front(IWYUPragmaExport))
+ if (Err)
return false;
- unsigned Offset = SM.getFileOffset(Range.getBegin());
- LastPragmaKeepInMainFileLine =
- SM.getLineNumber(SM.getFileID(Range.getBegin()), Offset) - 1;
+ if (inMainFile()) {
+ // Given:
+ //
+ // #include "foo.h"
+ // #include "bar.h" // IWYU pragma: keep
+ //
+ // The order in which the callbacks will be triggered:
+ //
+ // 1. InclusionDirective("foo.h")
+ // 2. handleCommentInMainFile("// IWYU pragma: keep")
+ // 3. InclusionDirective("bar.h")
+ //
+ // This code stores the last location of "IWYU pragma: keep" (or export)
+ // comment in the main file, so that when InclusionDirective is called, it
+ // will know that the next inclusion is behind the IWYU pragma.
+ // FIXME: Support "IWYU pragma: begin_exports" and "IWYU pragma:
+ // end_exports".
+ if (!Text.startswith(IWYUPragmaExport) &&
+ !Text.startswith(IWYUPragmaKeep))
+ return false;
+ unsigned Offset = SM.getFileOffset(Range.getBegin());
+ LastPragmaKeepInMainFileLine =
+ SM.getLineNumber(SM.getMainFileID(), Offset) - 1;
+ } else {
+ // Memorize headers that that have export pragmas in them. Include Cleaner
+ // does not support them properly yet, so they will be not marked as
+ // unused.
+ // FIXME: Once IncludeCleaner supports export pragmas, remove this.
+ if (!Text.startswith(IWYUPragmaExport) &&
+ !Text.startswith(IWYUPragmaBeginExports))
+ return false;
+ Out->HasIWYUPragmas.insert(
+ *Out->getID(SM.getFileEntryForID(SM.getFileID(Range.getBegin()))));
+ }
return false;
}
@@ -237,8 +252,7 @@
return It->second;
}
-IncludeStructure::HeaderID
-IncludeStructure::getOrCreateID(FileEntryRef Entry) {
+IncludeStructure::HeaderID IncludeStructure::getOrCreateID(FileEntryRef Entry) {
// Main file's FileEntry was not known at IncludeStructure creation time.
if (&Entry.getFileEntry() == MainFileEntry) {
if (RealPathNames.front().empty())
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
