zyounan updated this revision to Diff 494911.
zyounan added a comment.
Herald added a subscriber: ormris.
Update tests && Use Preprocessor.LookupFile
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143319/new/
https://reviews.llvm.org/D143319
Files:
clang-tools-extra/clangd/CodeComplete.cpp
clang-tools-extra/clangd/Headers.cpp
clang-tools-extra/clangd/Headers.h
clang-tools-extra/clangd/IncludeCleaner.cpp
clang-tools-extra/clangd/IncludeCleaner.h
clang-tools-extra/clangd/ParsedAST.cpp
clang-tools-extra/clangd/index/CanonicalIncludes.h
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
@@ -9,6 +9,7 @@
#include "Annotations.h"
#include "Config.h"
#include "IncludeCleaner.h"
+#include "Protocol.h"
#include "SourceCode.h"
#include "TestFS.h"
#include "TestTU.h"
@@ -22,8 +23,10 @@
namespace clangd {
namespace {
+using ::testing::AllOf;
using ::testing::ElementsAre;
using ::testing::ElementsAreArray;
+using ::testing::Field;
using ::testing::IsEmpty;
using ::testing::Pointee;
using ::testing::UnorderedElementsAre;
@@ -611,6 +614,36 @@
EXPECT_THAT(computeUnusedIncludesExperimental(AST), IsEmpty());
}
+MATCHER_P(message, Message, "") { return arg.Message == Message; }
+MATCHER_P2(range, Start, End, "") {
+ return arg.start == Start && arg.end == End;
+}
+
+TEST(IncludeCleaner, IWYUPragmaNoInclude) {
+ TestTU TU;
+ TU.Code = R"cpp(
+#include "foo.h"
+#include "bar.h"
+// IWYU pragma: no_include "bar.h"
+ )cpp";
+ TU.AdditionalFiles["foo.h"] = guard("");
+ TU.AdditionalFiles["bar.h"] = guard("");
+ ParsedAST AST = TU.build();
+
+ auto Diags = issueNoIncludesDiagnostics(AST, TU.Code);
+ EXPECT_THAT(
+ Diags,
+ UnorderedElementsAre(AllOf(
+ message(
+ "included header 'bar.h' is marked as `no_include` at line 4"),
+ Field(&DiagBase::Range, range(Position{2, 0}, Position{2, 16})))));
+ ASSERT_TRUE(Diags.size() == 1);
+ ASSERT_TRUE(Diags.back().Fixes.size() == 1);
+ ASSERT_TRUE(Diags.back().Fixes.back().Edits.size() == 1);
+ EXPECT_THAT(Diags.back().Fixes.back().Edits.back().range,
+ range(Position{2, 0}, Position{3, 0}));
+}
+
TEST(IncludeCleaner, NoDiagsForObjC) {
TestTU TU;
TU.Code = R"cpp(
Index: clang-tools-extra/clangd/unittests/HeadersTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/HeadersTests.cpp
+++ clang-tools-extra/clangd/unittests/HeadersTests.cpp
@@ -93,7 +93,8 @@
// Calculates the include path, or returns "" on error or header should not be
// inserted.
std::string calculate(PathRef Original, PathRef Preferred = "",
- const std::vector<Inclusion> &Inclusions = {}) {
+ const std::vector<Inclusion> &Inclusions = {},
+ const std::vector<IwyuNoInclude> &IwyuNoIncludes = {}) {
Clang = setupClang();
PreprocessOnlyAction Action;
EXPECT_TRUE(
@@ -111,6 +112,8 @@
&Clang->getPreprocessor().getHeaderSearchInfo());
for (const auto &Inc : Inclusions)
Inserter.addExisting(Inc);
+ for (const auto &Inc : IwyuNoIncludes)
+ Inserter.addNoInclude(Inc);
auto Inserted = ToHeaderFile(Preferred);
if (!Inserter.shouldInsertInclude(Original, Inserted))
return "";
@@ -275,6 +278,47 @@
AllOf(written("\"bar.h\""), hasPragmaKeep(true))));
}
+TEST_F(HeadersTest, IWYUPragmaNoInclude) {
+ FS.Files[MainFile] = R"cpp(
+ // IWYU pragma: no_include "baz.h"
+ #include "foo.h"
+ // IWYU pragma: no_include <bar.h>
+ // IWYU pragma: no_include <multiple> "headers.hpp" are not <valid>
+ // IWYU pragma: no_include <multiple> "headers.hpp" <invalid>
+ // IWYU pragma: no_include "multiple" <headers.hpp> "invalid"
+ // IWYU pragma: no_include no_quotes are considered invalid
+ // IWYU pragma: no_include <valid> texts after are ignored
+ // IWYU pragma: no_include not starting with quotes <are> "ignored"
+ )cpp";
+ FS.Files["sub/foo.h"] = "";
+ FS.Files["sub/bar.h"] = "";
+ FS.Files["sub/baz.h"] = "";
+ FS.Files["sub/valid"] = "";
+ FS.Files["sub/invalid"] = "";
+ FS.Files["sub/multiple"] = "";
+ FS.Files["sub/are"] = "";
+ FS.Files["sub/ignored"] = "";
+ FS.Files["sub/headers.hpp"] = "";
+
+ auto NI = collectIncludes().IwyuNoIncludes;
+ ASSERT_TRUE(NI);
+ EXPECT_THAT(*NI,
+ UnorderedElementsAre(
+ AllOf(written("\"baz.h\""),
+ resolved("/clangd-test/sub/baz.h"), includeLine(1)),
+ AllOf(written("<bar.h>"), resolved("/clangd-test/sub/bar.h"),
+ includeLine(3)),
+ AllOf(written("<valid>"), resolved("/clangd-test/sub/valid"),
+ includeLine(8))));
+ IwyuNoInclude Inc;
+ Inc.Written = "\"baz.h\"";
+ EXPECT_THAT(calculate("\"baz.h\"", "", {}, {Inc}), "");
+ Inc.Written = "<bar.h>";
+ EXPECT_THAT(calculate("<bar.h>", "", {}, {Inc}), "");
+ Inc.Written = "<valid>";
+ EXPECT_THAT(calculate("<valid>", "", {}, {Inc}), "");
+}
+
TEST_F(HeadersTest, InsertInclude) {
std::string Path = testPath("sub/bar.h");
FS.Files[Path] = "";
Index: clang-tools-extra/clangd/index/CanonicalIncludes.h
===================================================================
--- clang-tools-extra/clangd/index/CanonicalIncludes.h
+++ clang-tools-extra/clangd/index/CanonicalIncludes.h
@@ -87,6 +87,8 @@
/// - friend: this is less common than private, has implementation difficulties,
/// and affects behavior in a limited scope.
/// - associated: extremely rare
+/// Note that support for some other IWYU pragmas (export, keep, no_include...)
+/// are implemented in IncludeStructure.
std::unique_ptr<CommentHandler>
collectIWYUHeaderMaps(CanonicalIncludes *Includes);
Index: clang-tools-extra/clangd/ParsedAST.cpp
===================================================================
--- clang-tools-extra/clangd/ParsedAST.cpp
+++ clang-tools-extra/clangd/ParsedAST.cpp
@@ -571,6 +571,10 @@
MainFileIncludes = Preamble->Includes.MainFileIncludes;
for (const auto &Inc : Preamble->Includes.MainFileIncludes)
Inserter->addExisting(Inc);
+ if (Preamble->Includes.IwyuNoIncludes) {
+ for (const auto &NoInc : *Preamble->Includes.IwyuNoIncludes)
+ Inserter->addNoInclude(NoInc);
+ }
}
// FIXME: Consider piping through ASTSignals to fetch this to handle the
// case where a header file contains ObjC decls but no #imports.
@@ -691,9 +695,14 @@
if (Result.Diags) {
auto UnusedHeadersDiags =
issueUnusedIncludesDiagnostics(Result, Inputs.Contents);
+ auto NoIncludeHeadersDiags =
+ issueNoIncludesDiagnostics(Result, Inputs.Contents);
Result.Diags->insert(Result.Diags->end(),
make_move_iterator(UnusedHeadersDiags.begin()),
make_move_iterator(UnusedHeadersDiags.end()));
+ Result.Diags->insert(Result.Diags->end(),
+ make_move_iterator(NoIncludeHeadersDiags.begin()),
+ make_move_iterator(NoIncludeHeadersDiags.end()));
}
return std::move(Result);
}
Index: clang-tools-extra/clangd/IncludeCleaner.h
===================================================================
--- clang-tools-extra/clangd/IncludeCleaner.h
+++ clang-tools-extra/clangd/IncludeCleaner.h
@@ -105,6 +105,9 @@
std::vector<Diag> issueUnusedIncludesDiagnostics(ParsedAST &AST,
llvm::StringRef Code);
+std::vector<Diag> issueNoIncludesDiagnostics(ParsedAST &AST,
+ llvm::StringRef Code);
+
/// Affects whether standard library includes should be considered for
/// removal. This is off by default for now due to implementation limitations:
/// - macros are not tracked
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===================================================================
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -576,5 +576,46 @@
return Result;
}
+std::vector<Diag> issueNoIncludesDiagnostics(ParsedAST &AST,
+ llvm::StringRef Code) {
+ // C/C++ only
+ if (AST.getLangOpts().ObjC)
+ return {};
+ std::vector<Diag> Result;
+ std::string FileName =
+ AST.getSourceManager()
+ .getFileEntryRefForID(AST.getSourceManager().getMainFileID())
+ ->getName()
+ .str();
+ auto &IS = AST.getIncludeStructure();
+ auto &NoIncludes = IS.IwyuNoIncludes;
+ if (!NoIncludes)
+ return Result;
+ for (auto &Inc : IS.MainFileIncludes) {
+ const auto It = NoIncludes->find(
+ IwyuNoInclude(Inc.Written, Inc.Resolved, Inc.HashLine));
+ if (It == NoIncludes->end())
+ continue;
+ Diag D;
+ D.Message = llvm::formatv(
+ "included header '{0}' is marked as `no_include` at line {1}",
+ StringRef(Inc.Written).trim("<>\""), It->HashLine + 1);
+ D.Name = "no-include-headers";
+ D.Source = Diag::DiagSource::Clangd;
+ D.File = FileName;
+ D.Severity = DiagnosticsEngine::Warning;
+ D.Tags.push_back(Unnecessary);
+ D.Range = getDiagnosticRange(Code, Inc.HashOffset);
+ D.Fixes.emplace_back();
+ D.Fixes.back().Message = "remove #include directive";
+ D.Fixes.back().Edits.emplace_back();
+ D.Fixes.back().Edits.back().range.start.line = Inc.HashLine;
+ D.Fixes.back().Edits.back().range.end.line = Inc.HashLine + 1;
+ D.InsideMainFile = true;
+ Result.push_back(std::move(D));
+ }
+ return Result;
+}
+
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/Headers.h
===================================================================
--- clang-tools-extra/clangd/Headers.h
+++ clang-tools-extra/clangd/Headers.h
@@ -78,6 +78,18 @@
llvm::raw_ostream &operator<<(llvm::raw_ostream &, const Inclusion &);
bool operator==(const Inclusion &LHS, const Inclusion &RHS);
+struct IwyuNoInclude {
+ std::string
+ Written; // For `IWYU pragma no_include <vector>`, it is `<vector>`.
+ Path Resolved; // Resolved path to written file.
+ int HashLine; // 0-based line number containing IWYU pragma
+ IwyuNoInclude() = default;
+ IwyuNoInclude(std::string Written, Path Resolved, int HashLine)
+ : Written(std::move(Written)), Resolved(std::move(Resolved)),
+ HashLine(HashLine) {}
+ friend bool operator<(const IwyuNoInclude &LHS, const IwyuNoInclude &RHS);
+};
+
// Contains information about one file in the build graph and its direct
// dependencies. Doesn't own the strings it references (IncludeGraph is
// self-contained).
@@ -176,6 +188,8 @@
StdlibHeaders;
std::vector<Inclusion> MainFileIncludes;
+ std::optional<std::set<IwyuNoInclude>>
+ IwyuNoIncludes; // IWYU pragma: no_include
// We reserve HeaderID(0) for the main file and will manually check for that
// in getID and getOrCreateID because the UniqueID is not stable when the
@@ -219,6 +233,8 @@
void addExisting(const Inclusion &Inc);
+ void addNoInclude(const IwyuNoInclude &Inc);
+
/// Checks whether to add an #include of the header into \p File.
/// An #include will not be added if:
/// - Either \p DeclaringHeader or \p InsertedHeader is already (directly)
@@ -258,7 +274,9 @@
StringRef BuildDir;
HeaderSearch *HeaderSearchInfo = nullptr;
llvm::StringSet<> IncludedHeaders; // Both written and resolved.
- tooling::HeaderIncludes Inserter; // Computers insertion replacement.
+ llvm::StringSet<> IwyuNoIncludeHeaders; // Iwyu: no_include headers, with
+ // written and resolved.
+ tooling::HeaderIncludes Inserter; // Computers insertion replacement.
};
} // namespace clangd
Index: clang-tools-extra/clangd/Headers.cpp
===================================================================
--- clang-tools-extra/clangd/Headers.cpp
+++ clang-tools-extra/clangd/Headers.cpp
@@ -147,11 +147,70 @@
// will know that the next inclusion is behind the IWYU pragma.
// FIXME: Support "IWYU pragma: begin_exports" and "IWYU pragma:
// end_exports".
- if (!Pragma->startswith("export") && !Pragma->startswith("keep"))
- return false;
unsigned Offset = SM.getFileOffset(Range.getBegin());
- LastPragmaKeepInMainFileLine =
+ const unsigned CurrentLine =
SM.getLineNumber(SM.getMainFileID(), Offset) - 1;
+ if (Pragma->startswith("export") || Pragma->startswith("keep"))
+ LastPragmaKeepInMainFileLine = CurrentLine;
+ else if (Pragma->consume_front("no_include")) {
+ auto Spelling =
+ Pragma->take_until([](char C) { return C == '\n'; }).trim();
+ if (Spelling.empty())
+ return false;
+ const auto SplitQuotedIncludeHeaders = [](llvm::StringRef Buffer) {
+ llvm::SmallVector<llvm::StringRef> Ret;
+ for (size_t Pos = 0, Last = 0; Pos != Buffer.size();) {
+ char Closing = 0;
+ switch (Buffer[Pos]) {
+ case '<':
+ Last = Pos;
+ Closing = '>';
+ break;
+ case '"':
+ Last = Pos++;
+ Closing = '"';
+ break;
+ default:
+ // Ignore non-quoted string
+ ++Pos;
+ continue;
+ }
+ while (Pos != Buffer.size() && Buffer[Pos] != Closing)
+ ++Pos;
+ if (Pos == Buffer.size())
+ break;
+ Ret.emplace_back(Buffer.substr(Last, Pos - Last + 1));
+ ++Pos;
+ }
+ return Ret;
+ };
+ const auto Headers = SplitQuotedIncludeHeaders(Spelling);
+ // Should we support multiple headers on one pragma? Or emit a warning?
+ if (Headers.size() != 1)
+ return false;
+ Spelling = Headers[0];
+ if (const auto Header = Spelling.trim("\"<>"); !Header.empty()) {
+ const auto FE = PP.LookupFile(
+ /*FilenameLoc=*/{}, /*Filename=*/Header,
+ /*isAngled=*/Spelling.starts_with("<"),
+ /*FromDir=*/nullptr, /*FromFile=*/nullptr,
+ /*CurDir=*/nullptr, /*SearchPath=*/nullptr,
+ /*RelativePath=*/nullptr,
+ /*SuggestedModule=*/nullptr, /*IsMapped=*/nullptr,
+ /*IsFrameworkFound=*/nullptr, /*SkipCache=*/false,
+ /*OpenFile=*/false, /*CacheFailures=*/false);
+ if (!FE)
+ return false;
+ auto &NI = Out->IwyuNoIncludes;
+ if (!NI)
+ NI.emplace();
+ NI->insert(IwyuNoInclude(
+ /*Written=*/Spelling.str(),
+ /*Resolved=*/
+ FE->getFileEntry().tryGetRealPathName().str(), CurrentLine));
+ }
+ return false;
+ }
} else {
// Memorize headers that have export pragmas in them. Include Cleaner
// does not support them properly yet, so they will be not marked as
@@ -300,6 +359,12 @@
IncludedHeaders.insert(Inc.Resolved);
}
+void IncludeInserter::addNoInclude(const IwyuNoInclude &Inc) {
+ IwyuNoIncludeHeaders.insert(Inc.Written);
+ if (!Inc.Resolved.empty())
+ IwyuNoIncludeHeaders.insert(Inc.Resolved);
+}
+
/// FIXME(ioeric): we might not want to insert an absolute include path if the
/// path is not shortened.
bool IncludeInserter::shouldInsertInclude(
@@ -312,7 +377,13 @@
auto Included = [&](llvm::StringRef Header) {
return IncludedHeaders.find(Header) != IncludedHeaders.end();
};
- return !Included(DeclaringHeader) && !Included(InsertedHeader.File);
+ auto IsIwyuNoInclude = [&](llvm::StringRef Header) {
+ return IwyuNoIncludeHeaders.find(Header) != IwyuNoIncludeHeaders.end();
+ };
+ auto ShouldInsert = [&](auto &Predicate) {
+ return !Predicate(DeclaringHeader) && !Predicate(InsertedHeader.File);
+ };
+ return ShouldInsert(Included) && ShouldInsert(IsIwyuNoInclude);
}
std::optional<std::string>
@@ -369,5 +440,10 @@
RHS.Resolved, RHS.Written);
}
+bool operator<(const IwyuNoInclude &LHS, const IwyuNoInclude &RHS) {
+ // We want only 1 pragma for the same header.
+ return LHS.Written < RHS.Written;
+}
+
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/CodeComplete.cpp
===================================================================
--- clang-tools-extra/clangd/CodeComplete.cpp
+++ clang-tools-extra/clangd/CodeComplete.cpp
@@ -1524,6 +1524,10 @@
&Recorder->CCSema->getPreprocessor().getHeaderSearchInfo());
for (const auto &Inc : Includes.MainFileIncludes)
Inserter->addExisting(Inc);
+ if (Includes.IwyuNoIncludes) {
+ for (const auto &Inc : *Includes.IwyuNoIncludes)
+ Inserter->addNoInclude(Inc);
+ }
// Most of the cost of file proximity is in initializing the FileDistance
// structures based on the observed includes, once per query. Conceptually
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
