kadircet updated this revision to Diff 271390.
kadircet marked 5 inline comments as done.
kadircet added a comment.
- Address comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81964/new/
https://reviews.llvm.org/D81964
Files:
clang-tools-extra/clangd/ParsedAST.cpp
clang-tools-extra/clangd/Preamble.cpp
clang-tools-extra/clangd/Preamble.h
clang-tools-extra/clangd/unittests/ParsedASTTests.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
@@ -488,6 +488,51 @@
EXPECT_EQ(DecompLoc.first, SM.getMainFileID());
EXPECT_EQ(SM.getLineNumber(DecompLoc.first, DecompLoc.second), 3U);
}
+
+TEST(PreamblePatch, ModifiedBounds) {
+ struct {
+ llvm::StringLiteral Baseline;
+ llvm::StringLiteral Modified;
+ } Cases[] = {
+ // Size increased
+ {
+ "",
+ R"cpp(
+ #define FOO
+ FOO)cpp",
+ },
+ // Stayed same
+ {"#define FOO", "#define BAR"},
+ // Got smaller
+ {
+ R"cpp(
+ #define FOO
+ #undef FOO)cpp",
+ "#define FOO"},
+ };
+
+ for (const auto &Case : Cases) {
+ auto TU = TestTU::withCode(Case.Baseline);
+ auto BaselinePreamble = TU.preamble();
+ ASSERT_TRUE(BaselinePreamble);
+
+ Annotations Modified(Case.Modified);
+ TU.Code = Modified.code().str();
+ MockFSProvider FSProvider;
+ auto PP = PreamblePatch::create(testPath(TU.Filename),
+ TU.inputs(FSProvider), *BaselinePreamble);
+
+ IgnoreDiagnostics Diags;
+ auto CI = buildCompilerInvocation(TU.inputs(FSProvider), Diags);
+ ASSERT_TRUE(CI);
+
+ const auto ExpectedBounds =
+ Lexer::ComputePreamble(Case.Modified, *CI->getLangOpts());
+ EXPECT_EQ(PP.modifiedBounds().Size, ExpectedBounds.Size);
+ EXPECT_EQ(PP.modifiedBounds().PreambleEndsAtStartOfLine,
+ ExpectedBounds.PreambleEndsAtStartOfLine);
+ }
+}
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
+++ clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
@@ -444,24 +444,76 @@
EXPECT_EQ(SkippedFiles[I].kind(), tok::header_name);
}
+ TU.AdditionalFiles["a.h"] = "";
+ TU.AdditionalFiles["b.h"] = "";
+ TU.AdditionalFiles["c.h"] = "";
// Make sure replay logic works with patched preambles.
- TU.Code = "";
- StoreDiags Diags;
+ llvm::StringLiteral Baseline = R"cpp(
+ #include "a.h"
+ #include "c.h")cpp";
MockFSProvider FS;
+ TU.Code = Baseline.str();
auto Inputs = TU.inputs(FS);
- auto CI = buildCompilerInvocation(Inputs, Diags);
- auto EmptyPreamble =
- buildPreamble(testPath(TU.Filename), *CI, Inputs, true, nullptr);
- ASSERT_TRUE(EmptyPreamble);
- TU.Code = "#include <a.h>";
+ auto BaselinePreamble = TU.preamble();
+ ASSERT_TRUE(BaselinePreamble);
+
+ // First make sure we don't crash on various modifications to the preamble.
+ llvm::StringLiteral Cases[] = {
+ // clang-format off
+ // New include in middle.
+ R"cpp(
+ #include "a.h"
+ #include "b.h"
+ #include "c.h")cpp",
+ // New include at top.
+ R"cpp(
+ #include "b.h"
+ #include "a.h"
+ #include "c.h")cpp",
+ // New include at bottom.
+ R"cpp(
+ #include "a.h"
+ #include "c.h"
+ #include "b.h")cpp",
+ // Same size with a missing include.
+ R"cpp(
+ #include "a.h"
+ #include "b.h")cpp",
+ // Smaller with no new includes.
+ R"cpp(
+ #include "a.h")cpp",
+ // Smaller with a new includes.
+ R"cpp(
+ #include "b.h")cpp",
+ // clang-format on
+ };
+ for (llvm::StringLiteral Case : Cases) {
+ TU.Code = Case.str();
+
+ IgnoreDiagnostics Diags;
+ auto CI = buildCompilerInvocation(TU.inputs(FS), Diags);
+ auto PatchedAST = ParsedAST::build(testPath(TU.Filename), TU.inputs(FS),
+ std::move(CI), {}, BaselinePreamble);
+ ASSERT_TRUE(PatchedAST);
+ EXPECT_TRUE(PatchedAST->getDiagnostics().empty());
+ }
+
+ // Then ensure correctness by making sure includes were seen only once.
+ // Note that we first see the includes from the patch, as preamble includes
+ // are replayed after exiting the built-in file.
Includes.clear();
+ TU.Code = R"cpp(
+ #include "a.h"
+ #include "b.h")cpp";
+ IgnoreDiagnostics Diags;
+ auto CI = buildCompilerInvocation(TU.inputs(FS), Diags);
auto PatchedAST = ParsedAST::build(testPath(TU.Filename), TU.inputs(FS),
- std::move(CI), {}, EmptyPreamble);
+ std::move(CI), {}, BaselinePreamble);
ASSERT_TRUE(PatchedAST);
- // Make sure includes were seen only once.
+ EXPECT_TRUE(PatchedAST->getDiagnostics().empty());
EXPECT_THAT(Includes,
ElementsAre(WithFileName(testPath("__preamble_patch__.h")),
- WithFileName("a.h")));
+ WithFileName("b.h"), WithFileName("a.h")));
}
TEST(ParsedASTTest, PatchesAdditionalIncludes) {
Index: clang-tools-extra/clangd/Preamble.h
===================================================================
--- clang-tools-extra/clangd/Preamble.h
+++ clang-tools-extra/clangd/Preamble.h
@@ -31,6 +31,7 @@
#include "support/Path.h"
#include "clang/Frontend/CompilerInvocation.h"
#include "clang/Frontend/PrecompiledPreamble.h"
+#include "clang/Lex/Lexer.h"
#include "clang/Tooling/CompilationDatabase.h"
#include "llvm/ADT/StringRef.h"
@@ -119,6 +120,9 @@
/// using the presumed-location mechanism.
std::vector<Inclusion> preambleIncludes() const;
+ /// Returns preamble bounds for the Modified.
+ PreambleBounds modifiedBounds() const { return ModifiedBounds; }
+
/// Returns textual patch contents.
llvm::StringRef text() const { return PatchContents; }
@@ -129,6 +133,7 @@
/// Includes that are present in both \p Baseline and \p Modified. Used for
/// patching includes of baseline preamble.
std::vector<Inclusion> PreambleIncludes;
+ PreambleBounds ModifiedBounds = {0, false};
};
/// Translates locations inside preamble patch to their main-file equivalent
Index: clang-tools-extra/clangd/Preamble.cpp
===================================================================
--- clang-tools-extra/clangd/Preamble.cpp
+++ clang-tools-extra/clangd/Preamble.cpp
@@ -217,6 +217,7 @@
struct ScannedPreamble {
std::vector<Inclusion> Includes;
std::vector<TextualPPDirective> TextualDirectives;
+ PreambleBounds Bounds = {0, false};
};
/// Scans the preprocessor directives in the preamble section of the file by
@@ -284,6 +285,7 @@
IncludeStructure Includes;
PP.addPPCallbacks(collectIncludeStructureCallback(SM, &Includes));
ScannedPreamble SP;
+ SP.Bounds = Bounds;
PP.addPPCallbacks(
std::make_unique<DirectiveCollector>(PP, SP.TextualDirectives));
if (llvm::Error Err = Action.Execute())
@@ -463,6 +465,7 @@
llvm::sys::path::append(PatchName, llvm::sys::path::parent_path(FileName),
PreamblePatchHeaderName);
PP.PatchFileName = PatchName.str().str();
+ PP.ModifiedBounds = ModifiedScan->Bounds;
llvm::raw_string_ostream Patch(PP.PatchContents);
// Set default filename for subsequent #line directives
@@ -548,6 +551,7 @@
PreamblePatch PreamblePatch::unmodified(const PreambleData &Preamble) {
PreamblePatch PP;
PP.PreambleIncludes = Preamble.Includes.MainFileIncludes;
+ PP.ModifiedBounds = Preamble.Preamble.getBounds();
return PP;
}
Index: clang-tools-extra/clangd/ParsedAST.cpp
===================================================================
--- clang-tools-extra/clangd/ParsedAST.cpp
+++ clang-tools-extra/clangd/ParsedAST.cpp
@@ -383,7 +383,7 @@
Includes.MainFileIncludes = Patch->preambleIncludes();
// Replay the preamble includes so that clang-tidy checks can see them.
ReplayPreamble::attach(Patch->preambleIncludes(), *Clang,
- Preamble->Preamble.getBounds());
+ Patch->modifiedBounds());
}
// Important: collectIncludeStructure is registered *after* ReplayPreamble!
// Otherwise we would collect the replayed includes again...
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
