Author: Kadir Cetinkaya Date: 2023-02-13T09:49:13+01:00 New Revision: fae01d175a29270ec01211d3988c7ae57ddabfd3
URL: https://github.com/llvm/llvm-project/commit/fae01d175a29270ec01211d3988c7ae57ddabfd3 DIFF: https://github.com/llvm/llvm-project/commit/fae01d175a29270ec01211d3988c7ae57ddabfd3.diff LOG: [clangd] Fix bugs in main-file include patching for stale preambles - Make sure main file includes are present even when they're not patched (because they didn't change or we're explicitly not patching them). - Populate extra fields for includes, which can be used by include-cleaner. Differential Revision: https://reviews.llvm.org/D143197 Added: Modified: clang-tools-extra/clangd/Preamble.cpp clang-tools-extra/clangd/unittests/PreambleTests.cpp Removed: ################################################################################ diff --git a/clang-tools-extra/clangd/Preamble.cpp b/clang-tools-extra/clangd/Preamble.cpp index 15eea9bb036bd..cdb8db14722d2 100644 --- a/clang-tools-extra/clangd/Preamble.cpp +++ b/clang-tools-extra/clangd/Preamble.cpp @@ -671,10 +671,10 @@ PreamblePatch PreamblePatch::create(llvm::StringRef FileName, // We are only interested in newly added includes, record the ones in // Baseline for exclusion. llvm::DenseMap<std::pair<tok::PPKeywordKind, llvm::StringRef>, - /*Resolved=*/llvm::StringRef> + const Inclusion *> ExistingIncludes; for (const auto &Inc : Baseline.Includes.MainFileIncludes) - ExistingIncludes[{Inc.Directive, Inc.Written}] = Inc.Resolved; + ExistingIncludes[{Inc.Directive, Inc.Written}] = &Inc; // There might be includes coming from disabled regions, record these for // exclusion too. note that we don't have resolved paths for those. for (const auto &Inc : BaselineScan->Includes) @@ -685,8 +685,13 @@ PreamblePatch PreamblePatch::create(llvm::StringRef FileName, // Include already present in the baseline preamble. Set resolved path and // put into preamble includes. if (It != ExistingIncludes.end()) { - Inc.Resolved = It->second.str(); - PP.PreambleIncludes.push_back(Inc); + auto &PatchedInc = PP.PreambleIncludes.emplace_back(); + // Copy everything from existing include, apart from the location, when + // it's coming from baseline preamble. + if (It->second) + PatchedInc = *It->second; + PatchedInc.HashLine = Inc.HashLine; + PatchedInc.HashOffset = Inc.HashOffset; continue; } // Include is new in the modified preamble. Inject it into the patch and @@ -696,6 +701,11 @@ PreamblePatch PreamblePatch::create(llvm::StringRef FileName, Patch << llvm::formatv( "#{0} {1}\n", spellingForIncDirective(Inc.Directive), Inc.Written); } + } else { + // Make sure we have the full set of includes available even when we're not + // patching. As these are used by features we provide afterwards like hover, + // go-to-def or include-cleaner when preamble is stale. + PP.PreambleIncludes = Baseline.Includes.MainFileIncludes; } if (DirectivesChanged) { diff --git a/clang-tools-extra/clangd/unittests/PreambleTests.cpp b/clang-tools-extra/clangd/unittests/PreambleTests.cpp index 1e95b62884f69..ac46bfd06a138 100644 --- a/clang-tools-extra/clangd/unittests/PreambleTests.cpp +++ b/clang-tools-extra/clangd/unittests/PreambleTests.cpp @@ -15,6 +15,7 @@ #include "TestFS.h" #include "TestTU.h" #include "XRefs.h" +#include "clang/Basic/SourceManager.h" #include "clang/Format/Format.h" #include "clang/Frontend/FrontendActions.h" #include "clang/Frontend/PrecompiledPreamble.h" @@ -23,6 +24,7 @@ #include "llvm/Support/MemoryBuffer.h" #include "llvm/Support/VirtualFileSystem.h" #include "gmock/gmock.h" +#include "gtest/gtest-matchers.h" #include "gtest/gtest.h" #include <memory> #include <optional> @@ -166,7 +168,7 @@ TEST(PreamblePatchTest, PatchesPreambleIncludes) { MockFS FS; IgnoreDiagnostics Diags; auto TU = TestTU::withCode(R"cpp( - #include "a.h" + #include "a.h" // IWYU pragma: keep #include "c.h" )cpp"); TU.AdditionalFiles["a.h"] = "#include \"b.h\""; @@ -185,9 +187,14 @@ TEST(PreamblePatchTest, PatchesPreambleIncludes) { *BaselinePreamble); // Only a.h should exists in the preamble, as c.h has been dropped and b.h was // newly introduced. - EXPECT_THAT(PP.preambleIncludes(), - ElementsAre(AllOf(Field(&Inclusion::Written, "\"a.h\""), - Field(&Inclusion::Resolved, testPath("a.h"))))); + EXPECT_THAT( + PP.preambleIncludes(), + ElementsAre(AllOf( + Field(&Inclusion::Written, "\"a.h\""), + Field(&Inclusion::Resolved, testPath("a.h")), + Field(&Inclusion::HeaderID, testing::Not(testing::Eq(std::nullopt))), + Field(&Inclusion::BehindPragmaKeep, true), + Field(&Inclusion::FileKind, SrcMgr::CharacteristicKind::C_User)))); } std::optional<ParsedAST> createPatchedAST(llvm::StringRef Baseline, @@ -225,6 +232,26 @@ std::string getPreamblePatch(llvm::StringRef Baseline, .str(); } +TEST(PreamblePatchTest, IncludesArePreserved) { + llvm::StringLiteral Baseline = R"(//error-ok +#include <foo> +#include <bar> +)"; + llvm::StringLiteral Modified = R"(//error-ok +#include <foo> +#include <bar> +#define FOO)"; + + auto Includes = createPatchedAST(Baseline, Modified.str()) + ->getIncludeStructure() + .MainFileIncludes; + EXPECT_TRUE(!Includes.empty()); + EXPECT_EQ(Includes, TestTU::withCode(Baseline) + .build() + .getIncludeStructure() + .MainFileIncludes); +} + TEST(PreamblePatchTest, Define) { // BAR should be defined while parsing the AST. struct { _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
