subject:"\[PATCH\] D75582\: \[clangd\] Track document versions, include them with diags, enhance logs"

[PATCH] D75582: [clangd] Track document versions, include them with diags, enhance logs

2020-03-05 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet added inline comments.



Comment at: clang-tools-extra/clangd/ClangdServer.h:73
 virtual void onDiagnosticsReady(PathRef File,
+const llvm::json::Value &Version,
 std::vector Diagnostics) {}

sammccall wrote:
> kadircet wrote:
> > can we rather have `Optional`s here(both for callbacks and 
> > `addDocument`)?
> > 
> > as clangdserver layer doesn't touch json objects at all currently.
> I really do want to make these opaque at the lower layer.
> json is a bit fiddly though, reworked to use strings instead.
looks better thanks !


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75582/new/

https://reviews.llvm.org/D75582



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75582: [clangd] Track document versions, include them with diags, enhance logs

2020-03-04 Thread Sam McCall via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rG2cd33e6fe60f: [clangd] Track document versions, include them 
with diags, enhance logs (authored by sammccall).

Changed prior to commit:
  https://reviews.llvm.org/D75582?vs=248351&id=248355#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75582/new/

https://reviews.llvm.org/D75582

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/Compiler.h
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/ParsedAST.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/index/FileIndex.cpp
  clang-tools-extra/clangd/index/FileIndex.h
  clang-tools-extra/clangd/test/diagnostic-category.test
  clang-tools-extra/clangd/test/diagnostics-no-tidy.test
  clang-tools-extra/clangd/test/diagnostics-notes.test
  clang-tools-extra/clangd/test/diagnostics.test
  clang-tools-extra/clangd/test/did-change-configuration-params.test
  clang-tools-extra/clangd/test/execute-command.test
  clang-tools-extra/clangd/test/fixits-codeaction.test
  clang-tools-extra/clangd/test/fixits-command.test
  clang-tools-extra/clangd/test/fixits-embed-in-diagnostic.test
  clang-tools-extra/clangd/test/path-mappings.test
  clang-tools-extra/clangd/test/semantic-highlighting.test
  clang-tools-extra/clangd/test/version.test
  clang-tools-extra/clangd/unittests/ClangdTests.cpp
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang-tools-extra/clangd/unittests/FileIndexTests.cpp
  clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
  clang-tools-extra/clangd/unittests/SyncAPI.cpp
  clang-tools-extra/clangd/unittests/SyncAPI.h
  clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp
  clang-tools-extra/clangd/unittests/XRefsTests.cpp

Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -849,7 +849,8 @@
   ElementsAre(Sym("foo.h", FooHeader.range(;
 
   // Only preamble is built, and no AST is built in this request.
-  Server.addDocument(FooCpp, FooWithoutHeader.code(), WantDiagnostics::No);
+  Server.addDocument(FooCpp, FooWithoutHeader.code(), "null",
+ WantDiagnostics::No);
   // We build AST here, and it should use the latest preamble rather than the
   // stale one.
   EXPECT_THAT(
@@ -859,7 +860,8 @@
   // Reset test environment.
   runAddDocument(Server, FooCpp, FooWithHeader.code());
   // Both preamble and AST are built in this request.
-  Server.addDocument(FooCpp, FooWithoutHeader.code(), WantDiagnostics::Yes);
+  Server.addDocument(FooCpp, FooWithoutHeader.code(), "null",
+ WantDiagnostics::Yes);
   // Use the AST being built in above request.
   EXPECT_THAT(
   cantFail(runLocateSymbolAt(Server, FooCpp, FooWithoutHeader.point())),
Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -97,7 +97,7 @@
 
 SymbolSlab TestTU::headerSymbols() const {
   auto AST = build();
-  return std::get<0>(indexHeaderSymbols(AST.getASTContext(),
+  return std::get<0>(indexHeaderSymbols(/*Version=*/"null", AST.getASTContext(),
 AST.getPreprocessorPtr(),
 AST.getCanonicalIncludes()));
 }
@@ -105,8 +105,8 @@
 std::unique_ptr TestTU::index() const {
   auto AST = build();
   auto Idx = std::make_unique(/*UseDex=*/true);
-  Idx->updatePreamble(Filename, AST.getASTContext(), AST.getPreprocessorPtr(),
-  AST.getCanonicalIncludes());
+  Idx->updatePreamble(Filename, /*Version=*/"null", AST.getASTContext(),
+  AST.getPreprocessorPtr(), AST.getCanonicalIncludes());
   Idx->updateMain(Filename, AST);
   return std::move(Idx);
 }
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -41,7 +41,15 @@
 using ::testing::UnorderedElementsAre;
 
 MATCHER_P2(TUState, State, ActionName, "") {
-  return arg.Action.S == State && arg.Action.Name == ActionName;
+  if (arg.Action.S != State) {
+*result_listener << "state is " << arg.Action.S;
+return false;
+  }
+  if (arg

[PATCH] D75582: [clangd] Track document versions, include them with diags, enhance logs

2020-03-04 Thread Sam McCall via Phabricator via cfe-commits

sammccall marked an inline comment as done.
sammccall added inline comments.



Comment at: clang-tools-extra/clangd/ClangdServer.h:73
 virtual void onDiagnosticsReady(PathRef File,
+const llvm::json::Value &Version,
 std::vector Diagnostics) {}

kadircet wrote:
> can we rather have `Optional`s here(both for callbacks and 
> `addDocument`)?
> 
> as clangdserver layer doesn't touch json objects at all currently.
I really do want to make these opaque at the lower layer.
json is a bit fiddly though, reworked to use strings instead.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75582/new/

https://reviews.llvm.org/D75582



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75582: [clangd] Track document versions, include them with diags, enhance logs

2020-03-04 Thread Sam McCall via Phabricator via cfe-commits

sammccall updated this revision to Diff 248351.
sammccall added a comment.

Use string instead of json Value


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75582/new/

https://reviews.llvm.org/D75582

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/Compiler.h
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/ParsedAST.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/index/FileIndex.cpp
  clang-tools-extra/clangd/index/FileIndex.h
  clang-tools-extra/clangd/test/diagnostic-category.test
  clang-tools-extra/clangd/test/diagnostics-no-tidy.test
  clang-tools-extra/clangd/test/diagnostics-notes.test
  clang-tools-extra/clangd/test/diagnostics.test
  clang-tools-extra/clangd/test/did-change-configuration-params.test
  clang-tools-extra/clangd/test/execute-command.test
  clang-tools-extra/clangd/test/fixits-codeaction.test
  clang-tools-extra/clangd/test/fixits-command.test
  clang-tools-extra/clangd/test/fixits-embed-in-diagnostic.test
  clang-tools-extra/clangd/test/path-mappings.test
  clang-tools-extra/clangd/test/semantic-highlighting.test
  clang-tools-extra/clangd/test/version.test
  clang-tools-extra/clangd/unittests/ClangdTests.cpp
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang-tools-extra/clangd/unittests/FileIndexTests.cpp
  clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
  clang-tools-extra/clangd/unittests/SyncAPI.cpp
  clang-tools-extra/clangd/unittests/SyncAPI.h
  clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp
  clang-tools-extra/clangd/unittests/XRefsTests.cpp

Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -849,7 +849,8 @@
   ElementsAre(Sym("foo.h", FooHeader.range(;
 
   // Only preamble is built, and no AST is built in this request.
-  Server.addDocument(FooCpp, FooWithoutHeader.code(), WantDiagnostics::No);
+  Server.addDocument(FooCpp, FooWithoutHeader.code(), "null",
+ WantDiagnostics::No);
   // We build AST here, and it should use the latest preamble rather than the
   // stale one.
   EXPECT_THAT(
@@ -859,7 +860,8 @@
   // Reset test environment.
   runAddDocument(Server, FooCpp, FooWithHeader.code());
   // Both preamble and AST are built in this request.
-  Server.addDocument(FooCpp, FooWithoutHeader.code(), WantDiagnostics::Yes);
+  Server.addDocument(FooCpp, FooWithoutHeader.code(), "null",
+ WantDiagnostics::Yes);
   // Use the AST being built in above request.
   EXPECT_THAT(
   cantFail(runLocateSymbolAt(Server, FooCpp, FooWithoutHeader.point())),
Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -97,16 +97,16 @@
 
 SymbolSlab TestTU::headerSymbols() const {
   auto AST = build();
-  return std::get<0>(indexHeaderSymbols(AST.getASTContext(),
-AST.getPreprocessorPtr(),
-AST.getCanonicalIncludes()));
+  return std::get<0>(
+  indexHeaderSymbols(/*Version=*/"null", AST.getASTContext(),
+ AST.getPreprocessorPtr(), AST.getCanonicalIncludes()));
 }
 
 std::unique_ptr TestTU::index() const {
   auto AST = build();
   auto Idx = std::make_unique(/*UseDex=*/true);
-  Idx->updatePreamble(Filename, AST.getASTContext(), AST.getPreprocessorPtr(),
-  AST.getCanonicalIncludes());
+  Idx->updatePreamble(Filename, /*Version=*/"null", AST.getASTContext(),
+  AST.getPreprocessorPtr(), AST.getCanonicalIncludes());
   Idx->updateMain(Filename, AST);
   return std::move(Idx);
 }
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -41,7 +41,15 @@
 using ::testing::UnorderedElementsAre;
 
 MATCHER_P2(TUState, State, ActionName, "") {
-  return arg.Action.S == State && arg.Action.Name == ActionName;
+  if (arg.Action.S != State) {
+*result_listener << "state is " << arg.Action.S;
+return false;
+  }
+  if (arg.Action.Name != ActionName) {
+*result_listener << "name is " << arg.Action.Name;
+return false;

[PATCH] D75582: [clangd] Track document versions, include them with diags, enhance logs

2020-03-04 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet added inline comments.



Comment at: clang-tools-extra/clangd/ClangdServer.h:73
 virtual void onDiagnosticsReady(PathRef File,
+const llvm::json::Value &Version,
 std::vector Diagnostics) {}

can we rather have `Optional`s here(both for callbacks and `addDocument`)?

as clangdserver layer doesn't touch json objects at all currently.



Comment at: clang-tools-extra/clangd/Compiler.h:49
+  // Version identifier for Contents, provided by the client and opaque to us.
+  llvm::json::Value Version = nullptr;
   // Prevent reuse of the cached preamble/AST. Slow! Useful to workaround

again this (and other clangdserver structs) can hold a optional right?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75582/new/

https://reviews.llvm.org/D75582



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75582: [clangd] Track document versions, include them with diags, enhance logs

2020-03-04 Thread Haojian Wu via Phabricator via cfe-commits

hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.

looks good, just a reminder -- the API change of the clangdServer::Callback 
would break out our internal integration.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75582/new/

https://reviews.llvm.org/D75582



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75582: [clangd] Track document versions, include them with diags, enhance logs

2020-03-03 Thread Sam McCall via Phabricator via cfe-commits

sammccall updated this revision to Diff 248070.
sammccall added a comment.

Fix incomplete message.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75582/new/

https://reviews.llvm.org/D75582

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/Compiler.h
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/ParsedAST.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/index/FileIndex.cpp
  clang-tools-extra/clangd/index/FileIndex.h
  clang-tools-extra/clangd/test/diagnostic-category.test
  clang-tools-extra/clangd/test/diagnostics-no-tidy.test
  clang-tools-extra/clangd/test/diagnostics-notes.test
  clang-tools-extra/clangd/test/diagnostics.test
  clang-tools-extra/clangd/test/did-change-configuration-params.test
  clang-tools-extra/clangd/test/execute-command.test
  clang-tools-extra/clangd/test/fixits-codeaction.test
  clang-tools-extra/clangd/test/fixits-command.test
  clang-tools-extra/clangd/test/fixits-embed-in-diagnostic.test
  clang-tools-extra/clangd/test/path-mappings.test
  clang-tools-extra/clangd/test/semantic-highlighting.test
  clang-tools-extra/clangd/test/version.test
  clang-tools-extra/clangd/unittests/ClangdTests.cpp
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang-tools-extra/clangd/unittests/FileIndexTests.cpp
  clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
  clang-tools-extra/clangd/unittests/SyncAPI.cpp
  clang-tools-extra/clangd/unittests/SyncAPI.h
  clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp
  clang-tools-extra/clangd/unittests/XRefsTests.cpp

Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -838,7 +838,8 @@
   ElementsAre(Sym("foo.h", FooHeader.range(;
 
   // Only preamble is built, and no AST is built in this request.
-  Server.addDocument(FooCpp, FooWithoutHeader.code(), WantDiagnostics::No);
+  Server.addDocument(FooCpp, FooWithoutHeader.code(), nullptr,
+ WantDiagnostics::No);
   // We build AST here, and it should use the latest preamble rather than the
   // stale one.
   EXPECT_THAT(
@@ -848,7 +849,8 @@
   // Reset test environment.
   runAddDocument(Server, FooCpp, FooWithHeader.code());
   // Both preamble and AST are built in this request.
-  Server.addDocument(FooCpp, FooWithoutHeader.code(), WantDiagnostics::Yes);
+  Server.addDocument(FooCpp, FooWithoutHeader.code(), nullptr,
+ WantDiagnostics::Yes);
   // Use the AST being built in above request.
   EXPECT_THAT(
   cantFail(runLocateSymbolAt(Server, FooCpp, FooWithoutHeader.point())),
Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -97,16 +97,16 @@
 
 SymbolSlab TestTU::headerSymbols() const {
   auto AST = build();
-  return std::get<0>(indexHeaderSymbols(AST.getASTContext(),
-AST.getPreprocessorPtr(),
-AST.getCanonicalIncludes()));
+  return std::get<0>(
+  indexHeaderSymbols(/*Version=*/nullptr, AST.getASTContext(),
+ AST.getPreprocessorPtr(), AST.getCanonicalIncludes()));
 }
 
 std::unique_ptr TestTU::index() const {
   auto AST = build();
   auto Idx = std::make_unique(/*UseDex=*/true);
-  Idx->updatePreamble(Filename, AST.getASTContext(), AST.getPreprocessorPtr(),
-  AST.getCanonicalIncludes());
+  Idx->updatePreamble(Filename, /*Version=*/nullptr, AST.getASTContext(),
+  AST.getPreprocessorPtr(), AST.getCanonicalIncludes());
   Idx->updateMain(Filename, AST);
   return std::move(Idx);
 }
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -40,7 +40,15 @@
 using ::testing::UnorderedElementsAre;
 
 MATCHER_P2(TUState, State, ActionName, "") {
-  return arg.Action.S == State && arg.Action.Name == ActionName;
+  if (arg.Action.S != State) {
+*result_listener << "state is " << arg.Action.S;
+return false;
+  }
+  if (arg.Action.Name != ActionName) {
+*result_listener << "name is " << arg.Action.Name;
+return false;
+  }

[PATCH] D75582: [clangd] Track document versions, include them with diags, enhance logs

2020-03-03 Thread Sam McCall via Phabricator via cfe-commits

sammccall updated this revision to Diff 248069.
sammccall added a comment.

Fix accidental default.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75582/new/

https://reviews.llvm.org/D75582

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/Compiler.h
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/ParsedAST.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/index/FileIndex.cpp
  clang-tools-extra/clangd/index/FileIndex.h
  clang-tools-extra/clangd/test/diagnostic-category.test
  clang-tools-extra/clangd/test/diagnostics-no-tidy.test
  clang-tools-extra/clangd/test/diagnostics-notes.test
  clang-tools-extra/clangd/test/diagnostics.test
  clang-tools-extra/clangd/test/did-change-configuration-params.test
  clang-tools-extra/clangd/test/execute-command.test
  clang-tools-extra/clangd/test/fixits-codeaction.test
  clang-tools-extra/clangd/test/fixits-command.test
  clang-tools-extra/clangd/test/fixits-embed-in-diagnostic.test
  clang-tools-extra/clangd/test/path-mappings.test
  clang-tools-extra/clangd/test/semantic-highlighting.test
  clang-tools-extra/clangd/test/version.test
  clang-tools-extra/clangd/unittests/ClangdTests.cpp
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang-tools-extra/clangd/unittests/FileIndexTests.cpp
  clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
  clang-tools-extra/clangd/unittests/SyncAPI.cpp
  clang-tools-extra/clangd/unittests/SyncAPI.h
  clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp
  clang-tools-extra/clangd/unittests/XRefsTests.cpp

Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -838,7 +838,8 @@
   ElementsAre(Sym("foo.h", FooHeader.range(;
 
   // Only preamble is built, and no AST is built in this request.
-  Server.addDocument(FooCpp, FooWithoutHeader.code(), WantDiagnostics::No);
+  Server.addDocument(FooCpp, FooWithoutHeader.code(), nullptr,
+ WantDiagnostics::No);
   // We build AST here, and it should use the latest preamble rather than the
   // stale one.
   EXPECT_THAT(
@@ -848,7 +849,8 @@
   // Reset test environment.
   runAddDocument(Server, FooCpp, FooWithHeader.code());
   // Both preamble and AST are built in this request.
-  Server.addDocument(FooCpp, FooWithoutHeader.code(), WantDiagnostics::Yes);
+  Server.addDocument(FooCpp, FooWithoutHeader.code(), nullptr,
+ WantDiagnostics::Yes);
   // Use the AST being built in above request.
   EXPECT_THAT(
   cantFail(runLocateSymbolAt(Server, FooCpp, FooWithoutHeader.point())),
Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -97,16 +97,16 @@
 
 SymbolSlab TestTU::headerSymbols() const {
   auto AST = build();
-  return std::get<0>(indexHeaderSymbols(AST.getASTContext(),
-AST.getPreprocessorPtr(),
-AST.getCanonicalIncludes()));
+  return std::get<0>(
+  indexHeaderSymbols(/*Version=*/nullptr, AST.getASTContext(),
+ AST.getPreprocessorPtr(), AST.getCanonicalIncludes()));
 }
 
 std::unique_ptr TestTU::index() const {
   auto AST = build();
   auto Idx = std::make_unique(/*UseDex=*/true);
-  Idx->updatePreamble(Filename, AST.getASTContext(), AST.getPreprocessorPtr(),
-  AST.getCanonicalIncludes());
+  Idx->updatePreamble(Filename, /*Version=*/nullptr, AST.getASTContext(),
+  AST.getPreprocessorPtr(), AST.getCanonicalIncludes());
   Idx->updateMain(Filename, AST);
   return std::move(Idx);
 }
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -40,7 +40,15 @@
 using ::testing::UnorderedElementsAre;
 
 MATCHER_P2(TUState, State, ActionName, "") {
-  return arg.Action.S == State && arg.Action.Name == ActionName;
+  if (arg.Action.S != State) {
+*result_listener << "state is " << arg.Action.S;
+return false;
+  }
+  if (arg.Action.Name != ActionName) {
+*result_listener << "name is " << arg.Action.Name;
+return false;
+  }

[PATCH] D75582: [clangd] Track document versions, include them with diags, enhance logs

2020-03-03 Thread Sam McCall via Phabricator via cfe-commits

sammccall created this revision.
Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman, jkorous, 
MaskRay, javed.absar, ilya-biryukov.
Herald added a project: clang.
sammccall updated this revision to Diff 248069.
sammccall added a comment.
sammccall added a reviewer: hokein.
sammccall updated this revision to Diff 248070.

Fix accidental default.


sammccall added a comment.

Fix incomplete message.


This ties to an LSP feature (diagnostic versioning) but really a lot
of the value is in being able to log what's happening with file versions
and queues more descriptively and clearly.

As such it's fairly invasive, for a logging patch :-\

Key decisions:

- at the LSP layer, we don't reqire the client to provide versions (LSP makes 
it mandatory but we never enforced it). If not provided, versions start at 0 
and increment. DraftStore handles this.
- don't propagate magically using contexts, but rather manually: addDocument -> 
ParseInputs -> (ParsedAST, Preamble, various callbacks) Context-propagation 
would hide the versions from ClangdServer, which would make producing good log 
messages hard
- within ClangdServer, treat versions as opaque and unordered. json::Value is a 
convenient type for this, and allows richer versions for embedders. They're 
"mandatory" but nullptr is a sensible default.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D75582

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/Compiler.h
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/ParsedAST.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/index/FileIndex.cpp
  clang-tools-extra/clangd/index/FileIndex.h
  clang-tools-extra/clangd/test/diagnostic-category.test
  clang-tools-extra/clangd/test/diagnostics-no-tidy.test
  clang-tools-extra/clangd/test/diagnostics-notes.test
  clang-tools-extra/clangd/test/diagnostics.test
  clang-tools-extra/clangd/test/did-change-configuration-params.test
  clang-tools-extra/clangd/test/execute-command.test
  clang-tools-extra/clangd/test/fixits-codeaction.test
  clang-tools-extra/clangd/test/fixits-command.test
  clang-tools-extra/clangd/test/fixits-embed-in-diagnostic.test
  clang-tools-extra/clangd/test/path-mappings.test
  clang-tools-extra/clangd/test/semantic-highlighting.test
  clang-tools-extra/clangd/test/version.test
  clang-tools-extra/clangd/unittests/ClangdTests.cpp
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang-tools-extra/clangd/unittests/FileIndexTests.cpp
  clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
  clang-tools-extra/clangd/unittests/SyncAPI.cpp
  clang-tools-extra/clangd/unittests/SyncAPI.h
  clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp
  clang-tools-extra/clangd/unittests/XRefsTests.cpp

Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -838,7 +838,8 @@
   ElementsAre(Sym("foo.h", FooHeader.range(;
 
   // Only preamble is built, and no AST is built in this request.
-  Server.addDocument(FooCpp, FooWithoutHeader.code(), WantDiagnostics::No);
+  Server.addDocument(FooCpp, FooWithoutHeader.code(), nullptr,
+ WantDiagnostics::No);
   // We build AST here, and it should use the latest preamble rather than the
   // stale one.
   EXPECT_THAT(
@@ -848,7 +849,8 @@
   // Reset test environment.
   runAddDocument(Server, FooCpp, FooWithHeader.code());
   // Both preamble and AST are built in this request.
-  Server.addDocument(FooCpp, FooWithoutHeader.code(), WantDiagnostics::Yes);
+  Server.addDocument(FooCpp, FooWithoutHeader.code(), nullptr,
+ WantDiagnostics::Yes);
   // Use the AST being built in above request.
   EXPECT_THAT(
   cantFail(runLocateSymbolAt(Server, FooCpp, FooWithoutHeader.point())),
Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -97,16 +97,16 @@
 
 SymbolSlab TestTU::headerSymbols() const {
   auto AST = build();
-  return std::get<0>(indexHeaderSymbols(AST.getASTContext(),
-AST.getPreprocessorPtr(),
-AST.getCanonicalIncludes()));
+  return std::get<0>(
+  indexHeaderSymbols(/*Version=*/nullptr, AST.getASTContext(),
+ AST.getPrep

[PATCH] D75582: [clangd] Track document versions, include them with diags, enhance logs

[PATCH] D75582: [clangd] Track document versions, include them with diags, enhance logs

[PATCH] D75582: [clangd] Track document versions, include them with diags, enhance logs

[PATCH] D75582: [clangd] Track document versions, include them with diags, enhance logs

[PATCH] D75582: [clangd] Track document versions, include them with diags, enhance logs

[PATCH] D75582: [clangd] Track document versions, include them with diags, enhance logs

[PATCH] D75582: [clangd] Track document versions, include them with diags, enhance logs

[PATCH] D75582: [clangd] Track document versions, include them with diags, enhance logs

[PATCH] D75582: [clangd] Track document versions, include them with diags, enhance logs

9 matches

Site Navigation

Mail list logo

Footer information