[PATCH] D143197: [clangd] Fix bugs in main-file include patching for stale preambles
This revision was automatically updated to reflect the committed changes.
Closed by commit rGfae01d175a29: [clangd] Fix bugs in main-file include
patching for stale preambles (authored by kadircet).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143197/new/
https://reviews.llvm.org/D143197
Files:
clang-tools-extra/clangd/Preamble.cpp
clang-tools-extra/clangd/unittests/PreambleTests.cpp
Index: clang-tools-extra/clangd/unittests/PreambleTests.cpp
===
--- clang-tools-extra/clangd/unittests/PreambleTests.cpp
+++ 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
#include
@@ -166,7 +168,7 @@
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 @@
*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(::Written, "\"a.h\""),
-Field(::Resolved, testPath("a.h");
+ EXPECT_THAT(
+ PP.preambleIncludes(),
+ ElementsAre(AllOf(
+ Field(::Written, "\"a.h\""),
+ Field(::Resolved, testPath("a.h")),
+ Field(::HeaderID, testing::Not(testing::Eq(std::nullopt))),
+ Field(::BehindPragmaKeep, true),
+ Field(::FileKind, SrcMgr::CharacteristicKind::C_User;
}
std::optional createPatchedAST(llvm::StringRef Baseline,
@@ -225,6 +232,26 @@
.str();
}
+TEST(PreamblePatchTest, IncludesArePreserved) {
+ llvm::StringLiteral Baseline = R"(//error-ok
+#include
+#include
+)";
+ llvm::StringLiteral Modified = R"(//error-ok
+#include
+#include
+#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 {
Index: clang-tools-extra/clangd/Preamble.cpp
===
--- clang-tools-extra/clangd/Preamble.cpp
+++ clang-tools-extra/clangd/Preamble.cpp
@@ -671,10 +671,10 @@
// We are only interested in newly added includes, record the ones in
// Baseline for exclusion.
llvm::DenseMap,
- /*Resolved=*/llvm::StringRef>
+ const Inclusion *>
ExistingIncludes;
for (const auto : Baseline.Includes.MainFileIncludes)
- ExistingIncludes[{Inc.Directive, Inc.Written}] = Inc.Resolved;
+ ExistingIncludes[{Inc.Directive, Inc.Written}] =
// 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 : BaselineScan->Includes)
@@ -685,8 +685,13 @@
// 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 = 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 @@
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) {
___
cfe-commits mailing list
[PATCH] D143197: [clangd] Fix bugs in main-file include patching for stale preambles
kadircet added inline comments. Comment at: clang-tools-extra/clangd/Preamble.cpp:691 +// it's coming from baseline preamble. +if (It->second) + PatchedInc = *It->second; sammccall wrote: > if It->second is null, then all the `#includes` of this header from the > baseline preamble were in disabled sections, so it's *very* likely this one > is too. > > I think we're better not pushing onto PP.PreambleIncludes at all in this > case, rather than pushing an unresolved one - this is how the > MainFileIncludes looks like when an `#include` is in a disabled section and > there's no patching happening. agreed. as discussed sent out D143597 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143197/new/ https://reviews.llvm.org/D143197 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D143197: [clangd] Fix bugs in main-file include patching for stale preambles
kadircet updated this revision to Diff 495884.
kadircet marked 2 inline comments as done.
kadircet added a comment.
- use rawstrings in test
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143197/new/
https://reviews.llvm.org/D143197
Files:
clang-tools-extra/clangd/Preamble.cpp
clang-tools-extra/clangd/unittests/PreambleTests.cpp
Index: clang-tools-extra/clangd/unittests/PreambleTests.cpp
===
--- clang-tools-extra/clangd/unittests/PreambleTests.cpp
+++ 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
#include
@@ -166,7 +168,7 @@
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 @@
*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(::Written, "\"a.h\""),
-Field(::Resolved, testPath("a.h");
+ EXPECT_THAT(
+ PP.preambleIncludes(),
+ ElementsAre(AllOf(
+ Field(::Written, "\"a.h\""),
+ Field(::Resolved, testPath("a.h")),
+ Field(::HeaderID, testing::Not(testing::Eq(std::nullopt))),
+ Field(::BehindPragmaKeep, true),
+ Field(::FileKind, SrcMgr::CharacteristicKind::C_User;
}
std::optional createPatchedAST(llvm::StringRef Baseline,
@@ -225,6 +232,26 @@
.str();
}
+TEST(PreamblePatchTest, IncludesArePreserved) {
+ llvm::StringLiteral Baseline = R"(//error-ok
+#include
+#include
+)";
+ llvm::StringLiteral Modified = R"(//error-ok
+#include
+#include
+#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 {
Index: clang-tools-extra/clangd/Preamble.cpp
===
--- clang-tools-extra/clangd/Preamble.cpp
+++ clang-tools-extra/clangd/Preamble.cpp
@@ -671,10 +671,10 @@
// We are only interested in newly added includes, record the ones in
// Baseline for exclusion.
llvm::DenseMap,
- /*Resolved=*/llvm::StringRef>
+ const Inclusion *>
ExistingIncludes;
for (const auto : Baseline.Includes.MainFileIncludes)
- ExistingIncludes[{Inc.Directive, Inc.Written}] = Inc.Resolved;
+ ExistingIncludes[{Inc.Directive, Inc.Written}] =
// 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 : BaselineScan->Includes)
@@ -685,8 +685,13 @@
// 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 = 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 @@
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) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
[PATCH] D143197: [clangd] Fix bugs in main-file include patching for stale preambles
sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.
Comment at: clang-tools-extra/clangd/Preamble.cpp:691
+// it's coming from baseline preamble.
+if (It->second)
+ PatchedInc = *It->second;
if It->second is null, then all the `#includes` of this header from the
baseline preamble were in disabled sections, so it's *very* likely this one is
too.
I think we're better not pushing onto PP.PreambleIncludes at all in this case,
rather than pushing an unresolved one - this is how the MainFileIncludes looks
like when an `#include` is in a disabled section and there's no patching
happening.
Comment at: clang-tools-extra/clangd/unittests/PreambleTests.cpp:236
+TEST(PreamblePatchTest, IncludesArePreserved) {
+ llvm::StringLiteral Baseline = "//error-ok\n#include \n#include
\n";
+ auto Modified = Baseline + "#define FOO\n";
rawstrings?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143197/new/
https://reviews.llvm.org/D143197
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D143197: [clangd] Fix bugs in main-file include patching for stale preambles
kadircet added inline comments.
Comment at: clang-tools-extra/clangd/Preamble.cpp:690
+// it's coming from baseline preamble.
+if (It->second) {
+ Inc.Resolved = It->second->Resolved;
sammccall wrote:
> why the null check?
as discussed offline, we might have includes coming from scanned contents whose
are not part of the preamble, e.g. because they're in a disabled PP region.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143197/new/
https://reviews.llvm.org/D143197
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D143197: [clangd] Fix bugs in main-file include patching for stale preambles
kadircet updated this revision to Diff 495790.
kadircet marked 4 inline comments as done.
kadircet retitled this revision from "[clangd] Patch includes even without any
changes" to "[clangd] Fix bugs in main-file include patching for stale
preambles".
kadircet edited the summary of this revision.
kadircet added a comment.
- Address comments & update commit message
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143197/new/
https://reviews.llvm.org/D143197
Files:
clang-tools-extra/clangd/Preamble.cpp
clang-tools-extra/clangd/unittests/PreambleTests.cpp
Index: clang-tools-extra/clangd/unittests/PreambleTests.cpp
===
--- clang-tools-extra/clangd/unittests/PreambleTests.cpp
+++ 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
#include
@@ -166,7 +168,7 @@
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 @@
*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(::Written, "\"a.h\""),
-Field(::Resolved, testPath("a.h");
+ EXPECT_THAT(
+ PP.preambleIncludes(),
+ ElementsAre(AllOf(
+ Field(::Written, "\"a.h\""),
+ Field(::Resolved, testPath("a.h")),
+ Field(::HeaderID, testing::Not(testing::Eq(std::nullopt))),
+ Field(::BehindPragmaKeep, true),
+ Field(::FileKind, SrcMgr::CharacteristicKind::C_User;
}
std::optional createPatchedAST(llvm::StringRef Baseline,
@@ -225,6 +232,20 @@
.str();
}
+TEST(PreamblePatchTest, IncludesArePreserved) {
+ llvm::StringLiteral Baseline = "//error-ok\n#include \n#include \n";
+ auto Modified = Baseline + "#define FOO\n";
+
+ 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 {
Index: clang-tools-extra/clangd/Preamble.cpp
===
--- clang-tools-extra/clangd/Preamble.cpp
+++ clang-tools-extra/clangd/Preamble.cpp
@@ -671,10 +671,10 @@
// We are only interested in newly added includes, record the ones in
// Baseline for exclusion.
llvm::DenseMap,
- /*Resolved=*/llvm::StringRef>
+ const Inclusion *>
ExistingIncludes;
for (const auto : Baseline.Includes.MainFileIncludes)
- ExistingIncludes[{Inc.Directive, Inc.Written}] = Inc.Resolved;
+ ExistingIncludes[{Inc.Directive, Inc.Written}] =
// 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 : BaselineScan->Includes)
@@ -685,8 +685,13 @@
// 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 = 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 @@
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 =
