dgoldman updated this revision to Diff 242155.
dgoldman added a comment.
- Fix comment
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73916/new/
https://reviews.llvm.org/D73916
Files:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/ClangdServer.h
clang-tools-extra/clangd/Preamble.cpp
clang-tools-extra/clangd/Preamble.h
clang-tools-extra/clangd/Protocol.cpp
clang-tools-extra/clangd/Protocol.h
clang-tools-extra/clangd/TUScheduler.cpp
clang-tools-extra/clangd/TUScheduler.h
clang-tools-extra/clangd/unittests/FileIndexTests.cpp
clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
clang-tools-extra/clangd/unittests/TestTU.cpp
Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -69,7 +69,8 @@
buildPreamble(FullFilename, *CI,
/*OldPreamble=*/nullptr,
/*OldCompileCommand=*/Inputs.CompileCommand, Inputs,
- /*StoreInMemory=*/true, /*PreambleCallback=*/nullptr);
+ /*StoreInMemory=*/true, /*ForceReload*/false,
+ /*PreambleCallback=*/nullptr);
auto AST =
buildAST(FullFilename, std::move(CI), Diags.take(), Inputs, Preamble);
if (!AST.hasValue()) {
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -61,7 +61,8 @@
llvm::StringRef Contents, WantDiagnostics WD,
llvm::unique_function<void()> CB) {
WithContextValue Ctx(llvm::make_scope_exit(std::move(CB)));
- S.update(File, getInputs(File, std::string(Contents)), WD);
+ S.update(File, getInputs(File, std::string(Contents)), WD,
+ /*ForceReload*/false);
}
static Key<llvm::unique_function<void(PathRef File, std::vector<Diag>)>>
@@ -101,7 +102,7 @@
/// any. The TUScheduler should be created with captureDiags as a
/// DiagsCallback for this to work.
void updateWithDiags(TUScheduler &S, PathRef File, ParseInputs Inputs,
- WantDiagnostics WD,
+ WantDiagnostics WD, bool ForceReload,
llvm::unique_function<void(std::vector<Diag>)> CB) {
Path OrigFile = File.str();
WithContextValue Ctx(DiagsCallbackKey,
@@ -110,14 +111,14 @@
assert(File == OrigFile);
CB(std::move(Diags));
});
- S.update(File, std::move(Inputs), WD);
+ S.update(File, std::move(Inputs), WD, ForceReload);
}
void updateWithDiags(TUScheduler &S, PathRef File, llvm::StringRef Contents,
- WantDiagnostics WD,
+ WantDiagnostics WD, bool ForceReload,
llvm::unique_function<void(std::vector<Diag>)> CB) {
return updateWithDiags(S, File, getInputs(File, std::string(Contents)), WD,
- std::move(CB));
+ ForceReload, std::move(CB));
}
llvm::StringMap<std::string> Files;
@@ -138,7 +139,8 @@
Files[Missing] = "";
EXPECT_EQ(S.getContents(Added), "");
- S.update(Added, getInputs(Added, "x"), WantDiagnostics::No);
+ S.update(Added, getInputs(Added, "x"), WantDiagnostics::No,
+ /*ForceReload*/false);
EXPECT_EQ(S.getContents(Added), "x");
// Assert each operation for missing file is an error (even if it's
@@ -182,20 +184,24 @@
Notification Ready;
TUScheduler S(CDB, optsForTest(), captureDiags());
auto Path = testPath("foo.cpp");
- updateWithDiags(S, Path, "", WantDiagnostics::Yes,
+ updateWithDiags(S, Path, "", WantDiagnostics::Yes, /*ForceReload*/false,
[&](std::vector<Diag>) { Ready.wait(); });
updateWithDiags(S, Path, "request diags", WantDiagnostics::Yes,
+ /*ForceReload*/false,
[&](std::vector<Diag>) { ++CallbackCount; });
updateWithDiags(S, Path, "auto (clobbered)", WantDiagnostics::Auto,
+ /*ForceReload*/false,
[&](std::vector<Diag>) {
ADD_FAILURE()
<< "auto should have been cancelled by auto";
});
updateWithDiags(S, Path, "request no diags", WantDiagnostics::No,
+ /*ForceReload*/false,
[&](std::vector<Diag>) {
ADD_FAILURE() << "no diags should not be called back";
});
updateWithDiags(S, Path, "auto (produces)", WantDiagnostics::Auto,
+ /*ForceReload*/false,
[&](std::vector<Diag>) { ++CallbackCount; });
Ready.notify();
@@ -213,15 +219,18 @@
// FIXME: we could probably use timeouts lower than 1 second here.
auto Path = testPath("foo.cpp");
updateWithDiags(S, Path, "auto (debounced)", WantDiagnostics::Auto,
+ /*ForceReload*/false,
[&](std::vector<Diag>) {
ADD_FAILURE()
<< "auto should have been debounced and canceled";
});
std::this_thread::sleep_for(std::chrono::milliseconds(200));
updateWithDiags(S, Path, "auto (timed out)", WantDiagnostics::Auto,
+ /*ForceReload*/false,
[&](std::vector<Diag>) { ++CallbackCount; });
std::this_thread::sleep_for(std::chrono::seconds(2));
updateWithDiags(S, Path, "auto (shut down)", WantDiagnostics::Auto,
+ /*ForceReload*/false,
[&](std::vector<Diag>) { ++CallbackCount; });
ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
@@ -259,7 +268,8 @@
// If the second read was stale, it would usually see A.
std::this_thread::sleep_for(std::chrono::milliseconds(100));
});
- S.update(Path, getInputs(Path, "#include <B>"), WantDiagnostics::Yes);
+ S.update(Path, getInputs(Path, "#include <B>"), WantDiagnostics::Yes,
+ /*ForceReload*/false);
S.runWithPreamble("StaleRead", Path, TUScheduler::Stale,
[&](Expected<InputsAndPreamble> Pre) {
@@ -301,7 +311,7 @@
auto T = cancelableTask();
WithContext C(std::move(T.first));
updateWithDiags(
- S, Path, "//" + ID, WantDiagnostics::Yes,
+ S, Path, "//" + ID, WantDiagnostics::Yes, /*ForceReload*/false,
[&, ID](std::vector<Diag> Diags) { DiagsSeen.push_back(ID); });
return std::move(T.second);
};
@@ -392,7 +402,7 @@
{
WithContextValue WithNonce(NonceKey, ++Nonce);
updateWithDiags(
- S, File, Inputs, WantDiagnostics::Auto,
+ S, File, Inputs, WantDiagnostics::Auto, /*ForceReload*/false,
[File, Nonce, &Mut, &TotalUpdates](std::vector<Diag>) {
EXPECT_THAT(Context::current().get(NonceKey), Pointee(Nonce));
@@ -510,7 +520,8 @@
int main() {}
)cpp";
auto WithEmptyPreamble = R"cpp(int main() {})cpp";
- S.update(Foo, getInputs(Foo, WithPreamble), WantDiagnostics::Auto);
+ S.update(Foo, getInputs(Foo, WithPreamble), WantDiagnostics::Auto,
+ /*ForceReload*/false);
S.runWithPreamble(
"getNonEmptyPreamble", Foo, TUScheduler::Stale,
[&](Expected<InputsAndPreamble> Preamble) {
@@ -523,7 +534,8 @@
ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
// Update the file which results in an empty preamble.
- S.update(Foo, getInputs(Foo, WithEmptyPreamble), WantDiagnostics::Auto);
+ S.update(Foo, getInputs(Foo, WithEmptyPreamble), WantDiagnostics::Auto,
+ /*ForceReload*/false);
// Wait for the preamble is being built.
ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
S.runWithPreamble(
@@ -550,7 +562,8 @@
constexpr int ReadsToSchedule = 10;
std::mutex PreamblesMut;
std::vector<const void *> Preambles(ReadsToSchedule, nullptr);
- S.update(Foo, getInputs(Foo, NonEmptyPreamble), WantDiagnostics::Auto);
+ S.update(Foo, getInputs(Foo, NonEmptyPreamble), WantDiagnostics::Auto,
+ /*ForceReload*/false);
for (int I = 0; I < ReadsToSchedule; ++I) {
S.runWithPreamble(
"test", Foo, TUScheduler::Stale,
@@ -585,6 +598,7 @@
std::atomic<bool> Updated(false);
Updated = false;
updateWithDiags(S, Source, Contents, WantDiagnostics::Yes,
+ /*ForceReload*/false,
[&Updated](std::vector<Diag>) { Updated = true; });
bool UpdateFinished = S.blockUntilIdle(timeoutSeconds(10));
if (!UpdateFinished)
@@ -615,6 +629,44 @@
ASSERT_FALSE(DoUpdate(OtherSourceContents));
}
+TEST_F(TUSchedulerTests, ForceReload) {
+ TUScheduler S(CDB, optsForTest(), captureDiags());
+
+ auto Source = testPath("foo.cpp");
+ auto Header = testPath("foo.h");
+
+ auto SourceContents = R"cpp(
+ #include "foo.h"
+ int b = a;
+ )cpp";
+
+ // Return value indicates if the updated callback was received.
+ auto DoUpdate = [&](std::string Contents, bool ForceReload) -> bool {
+ std::atomic<bool> Updated(false);
+ Updated = false;
+ updateWithDiags(S, Source, Contents, WantDiagnostics::Yes, ForceReload,
+ [&Updated](std::vector<Diag>) { Updated = true; });
+ bool UpdateFinished = S.blockUntilIdle(timeoutSeconds(10));
+ if (!UpdateFinished)
+ ADD_FAILURE() << "Updated has not finished in one second. Threading bug?";
+ return Updated;
+ };
+
+ // Update the source contents, which should trigger an initial build with
+ // the header file missing.
+ ASSERT_TRUE(DoUpdate(SourceContents, /*ForceReload*/false));
+
+ // Add the header file.
+ Files[Header] = "int a;";
+ Timestamps[Header] = time_t(0);
+
+ // The addition of the missing header file shouldn't trigger a rebuild since
+ // we don't track missing files.
+ ASSERT_FALSE(DoUpdate(SourceContents, /*ForceReload*/false));
+
+ // Forcing the reload should should cause a rebuild, though.
+ ASSERT_TRUE(DoUpdate(SourceContents, /*ForceReload*/true));
+}
TEST_F(TUSchedulerTests, NoChangeDiags) {
TUScheduler S(CDB, optsForTest(), captureDiags());
@@ -622,7 +674,7 @@
auto Contents = "int a; int b;";
updateWithDiags(
- S, FooCpp, Contents, WantDiagnostics::No,
+ S, FooCpp, Contents, WantDiagnostics::No, /*ForceReload*/false,
[](std::vector<Diag>) { ADD_FAILURE() << "Should not be called."; });
S.runWithAST("touchAST", FooCpp, [](Expected<InputsAndAST> IA) {
// Make sure the AST was actually built.
@@ -634,6 +686,7 @@
// report the diagnostics, as they were not reported previously.
std::atomic<bool> SeenDiags(false);
updateWithDiags(S, FooCpp, Contents, WantDiagnostics::Auto,
+ /*ForceReload*/false,
[&](std::vector<Diag>) { SeenDiags = true; });
ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
ASSERT_TRUE(SeenDiags);
@@ -641,7 +694,7 @@
// Subsequent request does not get any diagnostics callback because the same
// diags have previously been reported and the inputs didn't change.
updateWithDiags(
- S, FooCpp, Contents, WantDiagnostics::Auto,
+ S, FooCpp, Contents, WantDiagnostics::Auto, /*ForceReload*/false,
[&](std::vector<Diag>) { ADD_FAILURE() << "Should not be called."; });
ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
}
@@ -721,7 +774,8 @@
TUScheduler S(CDB, optsForTest(), captureDiags());
std::vector<Diag> Diagnostics;
updateWithDiags(S, testPath("foo.cpp"), "void test() {}",
- WantDiagnostics::Yes, [&](std::vector<Diag> D) {
+ WantDiagnostics::Yes, /*ForceReload*/false,
+ [&](std::vector<Diag> D) {
Diagnostics = std::move(D);
Ready.notify();
});
@@ -745,7 +799,8 @@
TUScheduler S(CDB, optsForTest(), captureDiags());
std::vector<Diag> Diagnostics;
updateWithDiags(S, testPath("foo.cpp"), "void test() {}",
- WantDiagnostics::Yes, [&](std::vector<Diag> D) {
+ WantDiagnostics::Yes, /*ForceReload*/false,
+ [&](std::vector<Diag> D) {
Diagnostics = std::move(D);
Ready.notify();
});
Index: clang-tools-extra/clangd/unittests/FileIndexTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/FileIndexTests.cpp
+++ clang-tools-extra/clangd/unittests/FileIndexTests.cpp
@@ -288,7 +288,7 @@
bool IndexUpdated = false;
buildPreamble(
FooCpp, *CI, /*OldPreamble=*/nullptr, tooling::CompileCommand(), PI,
- /*StoreInMemory=*/true,
+ /*StoreInMemory=*/true, /*ForceReload*/false,
[&](ASTContext &Ctx, std::shared_ptr<Preprocessor> PP,
const CanonicalIncludes &CanonIncludes) {
EXPECT_FALSE(IndexUpdated) << "Expected only a single index update";
Index: clang-tools-extra/clangd/TUScheduler.h
===================================================================
--- clang-tools-extra/clangd/TUScheduler.h
+++ clang-tools-extra/clangd/TUScheduler.h
@@ -183,8 +183,10 @@
/// If diagnostics are requested (Yes), and the context is cancelled
/// before they are prepared, they may be skipped if eventual-consistency
/// permits it (i.e. WantDiagnostics is downgraded to Auto).
+ /// If ForceReload is true, then the preamble will be rebuilt.
/// Returns true if the file was not previously tracked.
- bool update(PathRef File, ParseInputs Inputs, WantDiagnostics WD);
+ bool update(PathRef File, ParseInputs Inputs, WantDiagnostics WD,
+ bool ForceReload);
/// Remove \p File from the list of tracked files and schedule removal of its
/// resources. Pending diagnostics for closed files may not be delivered, even
Index: clang-tools-extra/clangd/TUScheduler.cpp
===================================================================
--- clang-tools-extra/clangd/TUScheduler.cpp
+++ clang-tools-extra/clangd/TUScheduler.cpp
@@ -180,7 +180,7 @@
bool StorePreamblesInMemory, ParsingCallbacks &Callbacks);
~ASTWorker();
- void update(ParseInputs Inputs, WantDiagnostics);
+ void update(ParseInputs Inputs, WantDiagnostics, bool ForceReload);
void
runWithAST(llvm::StringRef Name,
llvm::unique_function<void(llvm::Expected<InputsAndAST>)> Action);
@@ -366,7 +366,8 @@
#endif
}
-void ASTWorker::update(ParseInputs Inputs, WantDiagnostics WantDiags) {
+void ASTWorker::update(ParseInputs Inputs, WantDiagnostics WantDiags,
+ bool ForceReload) {
llvm::StringRef TaskName = "Update";
auto Task = [=]() mutable {
auto RunPublish = [&](llvm::function_ref<void()> Publish) {
@@ -435,7 +436,7 @@
getPossiblyStalePreamble();
std::shared_ptr<const PreambleData> NewPreamble = buildPreamble(
FileName, *Invocation, OldPreamble, OldCommand, Inputs,
- StorePreambleInMemory,
+ StorePreambleInMemory, ForceReload,
[this](ASTContext &Ctx, std::shared_ptr<clang::Preprocessor> PP,
const CanonicalIncludes &CanonIncludes) {
Callbacks.onPreambleAST(FileName, Ctx, std::move(PP), CanonIncludes);
@@ -884,7 +885,7 @@
}
bool TUScheduler::update(PathRef File, ParseInputs Inputs,
- WantDiagnostics WantDiags) {
+ WantDiagnostics WantDiags, bool ForceReload) {
std::unique_ptr<FileData> &FD = Files[File];
bool NewFile = FD == nullptr;
if (!FD) {
@@ -898,7 +899,7 @@
} else {
FD->Contents = Inputs.Contents;
}
- FD->Worker->update(std::move(Inputs), WantDiags);
+ FD->Worker->update(std::move(Inputs), WantDiags, ForceReload);
return NewFile;
}
Index: clang-tools-extra/clangd/Protocol.h
===================================================================
--- clang-tools-extra/clangd/Protocol.h
+++ clang-tools-extra/clangd/Protocol.h
@@ -652,6 +652,13 @@
/// either they will be provided for this version or some subsequent one.
/// This is a clangd extension.
llvm::Optional<bool> wantDiagnostics;
+
+ /// Force the preamble to be rebuilt for this version of the file (only when
+ /// present and set to true). This is a workaround for how our heuristics for
+ /// preamble rebuilds will not detect added header files which were previously
+ /// missing.
+ /// This is a clangd extension.
+ llvm::Optional<bool> forceReload;
};
bool fromJSON(const llvm::json::Value &, DidChangeTextDocumentParams &);
Index: clang-tools-extra/clangd/Protocol.cpp
===================================================================
--- clang-tools-extra/clangd/Protocol.cpp
+++ clang-tools-extra/clangd/Protocol.cpp
@@ -432,7 +432,8 @@
llvm::json::ObjectMapper O(Params);
return O && O.map("textDocument", R.textDocument) &&
O.map("contentChanges", R.contentChanges) &&
- O.map("wantDiagnostics", R.wantDiagnostics);
+ O.map("wantDiagnostics", R.wantDiagnostics) &&
+ O.map("forceReload", R.forceReload);
}
bool fromJSON(const llvm::json::Value &E, FileChangeType &Out) {
Index: clang-tools-extra/clangd/Preamble.h
===================================================================
--- clang-tools-extra/clangd/Preamble.h
+++ clang-tools-extra/clangd/Preamble.h
@@ -78,7 +78,7 @@
buildPreamble(PathRef FileName, CompilerInvocation &CI,
std::shared_ptr<const PreambleData> OldPreamble,
const tooling::CompileCommand &OldCompileCommand,
- const ParseInputs &Inputs, bool StoreInMemory,
+ const ParseInputs &Inputs, bool StoreInMemory, bool ForceReload,
PreambleParsedCallback PreambleCallback);
Index: clang-tools-extra/clangd/Preamble.cpp
===================================================================
--- clang-tools-extra/clangd/Preamble.cpp
+++ clang-tools-extra/clangd/Preamble.cpp
@@ -89,7 +89,7 @@
buildPreamble(PathRef FileName, CompilerInvocation &CI,
std::shared_ptr<const PreambleData> OldPreamble,
const tooling::CompileCommand &OldCompileCommand,
- const ParseInputs &Inputs, bool StoreInMemory,
+ const ParseInputs &Inputs, bool StoreInMemory, bool ForceReload,
PreambleParsedCallback PreambleCallback) {
// Note that we don't need to copy the input contents, preamble can live
// without those.
@@ -98,14 +98,15 @@
auto Bounds =
ComputePreambleBounds(*CI.getLangOpts(), ContentsBuffer.get(), 0);
- if (OldPreamble &&
+ if (!ForceReload && OldPreamble &&
compileCommandsAreEqual(Inputs.CompileCommand, OldCompileCommand) &&
OldPreamble->Preamble.CanReuse(CI, ContentsBuffer.get(), Bounds,
Inputs.FS.get())) {
vlog("Reusing preamble for {0}", FileName);
return OldPreamble;
}
- vlog(OldPreamble ? "Rebuilding invalidated preamble for {0}"
+ vlog(OldPreamble ? (ForceReload ? "Rebuilding force-reloaded preamble for {0}"
+ : "Rebuilding invalidated preamble for {0}")
: "Building first preamble for {0}",
FileName);
Index: clang-tools-extra/clangd/ClangdServer.h
===================================================================
--- clang-tools-extra/clangd/ClangdServer.h
+++ clang-tools-extra/clangd/ClangdServer.h
@@ -172,7 +172,8 @@
/// separate thread. When the parsing is complete, DiagConsumer passed in
/// constructor will receive onDiagnosticsReady callback.
void addDocument(PathRef File, StringRef Contents,
- WantDiagnostics WD = WantDiagnostics::Auto);
+ WantDiagnostics WD = WantDiagnostics::Auto,
+ bool ForceReload = false);
/// Get the contents of \p File, which should have been added.
llvm::StringRef getDocument(PathRef File) const;
Index: clang-tools-extra/clangd/ClangdServer.cpp
===================================================================
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -170,7 +170,7 @@
}
void ClangdServer::addDocument(PathRef File, llvm::StringRef Contents,
- WantDiagnostics WantDiags) {
+ WantDiagnostics WantDiags, bool ForceReload) {
auto FS = FSProvider.getFileSystem();
ParseOptions Opts;
@@ -186,7 +186,7 @@
Inputs.Contents = std::string(Contents);
Inputs.Opts = std::move(Opts);
Inputs.Index = Index;
- bool NewFile = WorkScheduler.update(File, Inputs, WantDiags);
+ bool NewFile = WorkScheduler.update(File, Inputs, WantDiags, ForceReload);
// If we loaded Foo.h, we want to make sure Foo.cpp is indexed.
if (NewFile && BackgroundIdx)
BackgroundIdx->boostRelated(File);
Index: clang-tools-extra/clangd/ClangdLSPServer.cpp
===================================================================
--- clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -633,6 +633,9 @@
if (Params.wantDiagnostics.hasValue())
WantDiags = Params.wantDiagnostics.getValue() ? WantDiagnostics::Yes
: WantDiagnostics::No;
+ auto ForceReload = false;
+ if (Params.forceReload.hasValue())
+ ForceReload = Params.forceReload.getValue() ? true : false;
PathRef File = Params.textDocument.uri.file();
llvm::Expected<std::string> Contents =
@@ -647,7 +650,7 @@
return;
}
- Server->addDocument(File, *Contents, WantDiags);
+ Server->addDocument(File, *Contents, WantDiags, ForceReload);
}
void ClangdLSPServer::onFileEvent(const DidChangeWatchedFilesParams &Params) {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
