[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Simon Marchi via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL325784: [clangd] DidChangeConfiguration Notification 
(authored by simark, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D39571?vs=135229=135407#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D39571

Files:
  clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
  clang-tools-extra/trunk/clangd/ClangdLSPServer.h
  clang-tools-extra/trunk/clangd/ClangdServer.cpp
  clang-tools-extra/trunk/clangd/ClangdServer.h
  clang-tools-extra/trunk/clangd/DraftStore.cpp
  clang-tools-extra/trunk/clangd/DraftStore.h
  clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.cpp
  clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.h
  clang-tools-extra/trunk/clangd/Protocol.cpp
  clang-tools-extra/trunk/clangd/Protocol.h
  clang-tools-extra/trunk/clangd/ProtocolHandlers.cpp
  clang-tools-extra/trunk/clangd/ProtocolHandlers.h
  clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp

Index: clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
@@ -7,6 +7,7 @@
 //
 //===--===//
 
+#include "Annotations.h"
 #include "ClangdLSPServer.h"
 #include "ClangdServer.h"
 #include "Matchers.h"
@@ -77,6 +78,43 @@
   VFSTag LastVFSTag = VFSTag();
 };
 
+/// For each file, record whether the last published diagnostics contained at
+/// least one error.
+class MultipleErrorCheckingDiagConsumer : public DiagnosticsConsumer {
+public:
+  void
+  onDiagnosticsReady(PathRef File,
+ Tagged Diagnostics) override {
+bool HadError = diagsContainErrors(Diagnostics.Value);
+
+std::lock_guard Lock(Mutex);
+LastDiagsHadError[File] = HadError;
+  }
+
+  /// Exposes all files consumed by onDiagnosticsReady in an unspecified order.
+  /// For each file, a bool value indicates whether the last diagnostics
+  /// contained an error.
+  std::vector> filesWithDiags() const {
+std::vector> Result;
+std::lock_guard Lock(Mutex);
+
+for (const auto  : LastDiagsHadError) {
+  Result.emplace_back(it.first(), it.second);
+}
+
+return Result;
+  }
+
+  void clear() {
+std::lock_guard Lock(Mutex);
+LastDiagsHadError.clear();
+  }
+
+private:
+  mutable std::mutex Mutex;
+  llvm::StringMap LastDiagsHadError;
+};
+
 /// Replaces all patterns of the form 0x123abc with spaces
 std::string replacePtrsInDump(std::string const ) {
   llvm::Regex RE("0x[0-9a-fA-F]+");
@@ -413,6 +451,72 @@
   EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags());
 }
 
+// Test ClangdServer.reparseOpenedFiles.
+TEST_F(ClangdVFSTest, ReparseOpenedFiles) {
+  Annotations FooSource(R"cpp(
+#ifdef MACRO
+$one[[static void bob() {}]]
+#else
+$two[[static void bob() {}]]
+#endif
+
+int main () { bo^b (); return 0; }
+)cpp");
+
+  Annotations BarSource(R"cpp(
+#ifdef MACRO
+this is an error
+#endif
+)cpp");
+
+  Annotations BazSource(R"cpp(
+int hello;
+)cpp");
+
+  MockFSProvider FS;
+  MockCompilationDatabase CDB;
+  MultipleErrorCheckingDiagConsumer DiagConsumer;
+  ClangdServer Server(CDB, DiagConsumer, FS,
+  /*AsyncThreadsCount=*/0,
+  /*StorePreamblesInMemory=*/true);
+
+  auto FooCpp = testPath("foo.cpp");
+  auto BarCpp = testPath("bar.cpp");
+  auto BazCpp = testPath("baz.cpp");
+
+  FS.Files[FooCpp] = "";
+  FS.Files[BarCpp] = "";
+  FS.Files[BazCpp] = "";
+
+  CDB.ExtraClangFlags = {"-DMACRO=1"};
+  Server.addDocument(FooCpp, FooSource.code());
+  Server.addDocument(BarCpp, BarSource.code());
+  Server.addDocument(BazCpp, BazSource.code());
+
+  EXPECT_THAT(DiagConsumer.filesWithDiags(),
+  UnorderedElementsAre(Pair(FooCpp, false), Pair(BarCpp, true),
+   Pair(BazCpp, false)));
+
+  auto Locations = runFindDefinitions(Server, FooCpp, FooSource.point());
+  EXPECT_TRUE(bool(Locations));
+  EXPECT_THAT(Locations->Value, ElementsAre(Location{URIForFile{FooCpp},
+ FooSource.range("one")}));
+
+  // Undefine MACRO, close baz.cpp.
+  CDB.ExtraClangFlags.clear();
+  DiagConsumer.clear();
+  Server.removeDocument(BazCpp);
+  Server.reparseOpenedFiles();
+
+  EXPECT_THAT(DiagConsumer.filesWithDiags(),
+  UnorderedElementsAre(Pair(FooCpp, false), Pair(BarCpp, false)));
+
+  Locations = runFindDefinitions(Server, FooCpp, FooSource.point());
+  EXPECT_TRUE(bool(Locations));
+  EXPECT_THAT(Locations->Value, ElementsAre(Location{URIForFile{FooCpp},
+ FooSource.range("two")}));
+}
+
 

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.

Thanks for fixing all the comments! LGTM!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D39571



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

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 135229.
simark added a comment.

New version

Address comments in https://reviews.llvm.org/D39571#1014237


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D39571

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/DraftStore.cpp
  clangd/DraftStore.h
  clangd/GlobalCompilationDatabase.cpp
  clangd/GlobalCompilationDatabase.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  unittests/clangd/ClangdTests.cpp

Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -7,6 +7,7 @@
 //
 //===--===//
 
+#include "Annotations.h"
 #include "ClangdLSPServer.h"
 #include "ClangdServer.h"
 #include "Matchers.h"
@@ -77,6 +78,43 @@
   VFSTag LastVFSTag = VFSTag();
 };
 
+/// For each file, record whether the last published diagnostics contained at
+/// least one error.
+class MultipleErrorCheckingDiagConsumer : public DiagnosticsConsumer {
+public:
+  void
+  onDiagnosticsReady(PathRef File,
+ Tagged Diagnostics) override {
+bool HadError = diagsContainErrors(Diagnostics.Value);
+
+std::lock_guard Lock(Mutex);
+LastDiagsHadError[File] = HadError;
+  }
+
+  /// Exposes all files consumed by onDiagnosticsReady in an unspecified order.
+  /// For each file, a bool value indicates whether the last diagnostics
+  /// contained an error.
+  std::vector> filesWithDiags() const {
+std::vector> Result;
+std::lock_guard Lock(Mutex);
+
+for (const auto  : LastDiagsHadError) {
+  Result.emplace_back(it.first(), it.second);
+}
+
+return Result;
+  }
+
+  void clear() {
+std::lock_guard Lock(Mutex);
+LastDiagsHadError.clear();
+  }
+
+private:
+  mutable std::mutex Mutex;
+  llvm::StringMap LastDiagsHadError;
+};
+
 /// Replaces all patterns of the form 0x123abc with spaces
 std::string replacePtrsInDump(std::string const ) {
   llvm::Regex RE("0x[0-9a-fA-F]+");
@@ -413,6 +451,72 @@
   EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags());
 }
 
+// Test ClangdServer.reparseOpenedFiles.
+TEST_F(ClangdVFSTest, ReparseOpenedFiles) {
+  Annotations FooSource(R"cpp(
+#ifdef MACRO
+$one[[static void bob() {}]]
+#else
+$two[[static void bob() {}]]
+#endif
+
+int main () { bo^b (); return 0; }
+)cpp");
+
+  Annotations BarSource(R"cpp(
+#ifdef MACRO
+this is an error
+#endif
+)cpp");
+
+  Annotations BazSource(R"cpp(
+int hello;
+)cpp");
+
+  MockFSProvider FS;
+  MockCompilationDatabase CDB;
+  MultipleErrorCheckingDiagConsumer DiagConsumer;
+  ClangdServer Server(CDB, DiagConsumer, FS,
+  /*AsyncThreadsCount=*/0,
+  /*StorePreamblesInMemory=*/true);
+
+  auto FooCpp = testPath("foo.cpp");
+  auto BarCpp = testPath("bar.cpp");
+  auto BazCpp = testPath("baz.cpp");
+
+  FS.Files[FooCpp] = "";
+  FS.Files[BarCpp] = "";
+  FS.Files[BazCpp] = "";
+
+  CDB.ExtraClangFlags = {"-DMACRO=1"};
+  Server.addDocument(FooCpp, FooSource.code());
+  Server.addDocument(BarCpp, BarSource.code());
+  Server.addDocument(BazCpp, BazSource.code());
+
+  EXPECT_THAT(DiagConsumer.filesWithDiags(),
+  UnorderedElementsAre(Pair(FooCpp, false), Pair(BarCpp, true),
+   Pair(BazCpp, false)));
+
+  auto Locations = runFindDefinitions(Server, FooCpp, FooSource.point());
+  EXPECT_TRUE(bool(Locations));
+  EXPECT_THAT(Locations->Value, ElementsAre(Location{URIForFile{FooCpp},
+ FooSource.range("one")}));
+
+  // Undefine MACRO, close baz.cpp.
+  CDB.ExtraClangFlags.clear();
+  DiagConsumer.clear();
+  Server.removeDocument(BazCpp);
+  Server.reparseOpenedFiles();
+
+  EXPECT_THAT(DiagConsumer.filesWithDiags(),
+  UnorderedElementsAre(Pair(FooCpp, false), Pair(BarCpp, false)));
+
+  Locations = runFindDefinitions(Server, FooCpp, FooSource.point());
+  EXPECT_TRUE(bool(Locations));
+  EXPECT_THAT(Locations->Value, ElementsAre(Location{URIForFile{FooCpp},
+ FooSource.range("two")}));
+}
+
 TEST_F(ClangdVFSTest, MemoryUsage) {
   MockFSProvider FS;
   ErrorCheckingDiagConsumer DiagConsumer;
Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -52,6 +52,7 @@
   virtual void onRename(RenameParams ) = 0;
   virtual void onDocumentHighlight(TextDocumentPositionParams ) = 0;
   virtual void onHover(TextDocumentPositionParams ) = 0;
+  virtual void onChangeConfiguration(DidChangeConfigurationParams ) = 0;
 };
 
 void registerCallbackHandlers(JSONRPCDispatcher , JSONOutput ,
Index: 

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Simon Marchi via Phabricator via cfe-commits]

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



Comment at: unittests/clangd/ClangdTests.cpp:492
+
+  EXPECT_TRUE(DiagConsumer.contains(FooCpp));
+  EXPECT_TRUE(DiagConsumer.contains(BarCpp));

simark wrote:
> ilya-biryukov wrote:
> > Maybe expose a copy of the map from DiagConsumer and check for all files in 
> > a single line?
> > 
> > ```
> > class MultipleErrorCHeckingDiagConsumer {
> >/// Exposes all files consumed by onDiagnosticsReady in an unspecified 
> > order.
> >/// For each file, a bool value indicates whether the last diagnostics 
> > contained an error.
> >std::vector> filesWithDiags() const { /* ... */ }
> > };
> > 
> > /// 
> > EXPECT_THAT(DiagConsumer.filesWithDiags(), 
> > UnorderedElementsAre(Pair(FooCpp, false), Pair(BarCpp, true), Pair(BazCpp, 
> > false));
> > ```
> > 
> > It would make the test more concise.
> Nice :)
It's also better because we don't have to explicitly check that Baz is not 
there.  So if some other file sneaks in the result for some reason and 
shouldn't be there, the test will now fail, where it wouldn't have previously.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D39571



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

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Simon Marchi via Phabricator via cfe-commits]

simark marked 5 inline comments as done.
simark added inline comments.



Comment at: unittests/clangd/ClangdTests.cpp:492
+
+  EXPECT_TRUE(DiagConsumer.contains(FooCpp));
+  EXPECT_TRUE(DiagConsumer.contains(BarCpp));

ilya-biryukov wrote:
> Maybe expose a copy of the map from DiagConsumer and check for all files in a 
> single line?
> 
> ```
> class MultipleErrorCHeckingDiagConsumer {
>/// Exposes all files consumed by onDiagnosticsReady in an unspecified 
> order.
>/// For each file, a bool value indicates whether the last diagnostics 
> contained an error.
>std::vector> filesWithDiags() const { /* ... */ }
> };
> 
> /// 
> EXPECT_THAT(DiagConsumer.filesWithDiags(), UnorderedElementsAre(Pair(FooCpp, 
> false), Pair(BarCpp, true), Pair(BazCpp, false));
> ```
> 
> It would make the test more concise.
Nice :)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D39571



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

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: unittests/clangd/ClangdTests.cpp:83
+// least one error.
+class MultipleErrorCHeckingDiagConsumer : public DiagnosticsConsumer {
+public:

NIT: misspelling: ErrorCHecking instead of ErrorChecking



Comment at: unittests/clangd/ClangdTests.cpp:94
+
+  bool contains(PathRef P) {
+std::lock_guard Lock(Mutex);

This function should be `const`.
(Would require making `Mutex` mutable, but that's fine)



Comment at: unittests/clangd/ClangdTests.cpp:99
+
+  bool lastHadError(PathRef P) {
+std::lock_guard Lock(Mutex);

This function should be const and assert that P is in the map.



Comment at: unittests/clangd/ClangdTests.cpp:111
+  std::mutex Mutex;
+  std::map LastDiagsHadError;
+};

Maybe replace `std::map` to `llvm::StringMap`?



Comment at: unittests/clangd/ClangdTests.cpp:492
+
+  EXPECT_TRUE(DiagConsumer.contains(FooCpp));
+  EXPECT_TRUE(DiagConsumer.contains(BarCpp));

Maybe expose a copy of the map from DiagConsumer and check for all files in a 
single line?

```
class MultipleErrorCHeckingDiagConsumer {
   /// Exposes all files consumed by onDiagnosticsReady in an unspecified order.
   /// For each file, a bool value indicates whether the last diagnostics 
contained an error.
   std::vector> filesWithDiags() const { /* ... */ }
};

/// 
EXPECT_THAT(DiagConsumer.filesWithDiags(), UnorderedElementsAre(Pair(FooCpp, 
false), Pair(BarCpp, true), Pair(BazCpp, false));
```

It would make the test more concise.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D39571



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

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 135153.
simark added a comment.

New version, take 2

The previous version contains the changes of already merged patches, I'm not
sure what I did wrong.  This is another try.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D39571

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/DraftStore.cpp
  clangd/DraftStore.h
  clangd/GlobalCompilationDatabase.cpp
  clangd/GlobalCompilationDatabase.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  unittests/clangd/ClangdTests.cpp

Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -7,6 +7,7 @@
 //
 //===--===//
 
+#include "Annotations.h"
 #include "ClangdLSPServer.h"
 #include "ClangdServer.h"
 #include "Matchers.h"
@@ -77,6 +78,39 @@
   VFSTag LastVFSTag = VFSTag();
 };
 
+// For each file, record whether the last published diagnostics contained at
+// least one error.
+class MultipleErrorCHeckingDiagConsumer : public DiagnosticsConsumer {
+public:
+  void
+  onDiagnosticsReady(PathRef File,
+ Tagged Diagnostics) override {
+bool HadError = diagsContainErrors(Diagnostics.Value);
+
+std::lock_guard Lock(Mutex);
+LastDiagsHadError[File] = HadError;
+  }
+
+  bool contains(PathRef P) {
+std::lock_guard Lock(Mutex);
+return LastDiagsHadError.find(P) != LastDiagsHadError.end();
+  }
+
+  bool lastHadError(PathRef P) {
+std::lock_guard Lock(Mutex);
+return LastDiagsHadError[P];
+  }
+
+  void clear() {
+std::lock_guard Lock(Mutex);
+LastDiagsHadError.clear();
+  }
+
+private:
+  std::mutex Mutex;
+  std::map LastDiagsHadError;
+};
+
 /// Replaces all patterns of the form 0x123abc with spaces
 std::string replacePtrsInDump(std::string const ) {
   llvm::Regex RE("0x[0-9a-fA-F]+");
@@ -413,6 +447,78 @@
   EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags());
 }
 
+// Test ClangdServer.reparseOpenedFiles.
+TEST_F(ClangdVFSTest, ReparseOpenedFiles) {
+  Annotations FooSource(R"cpp(
+#ifdef MACRO
+$one[[static void bob() {}]]
+#else
+$two[[static void bob() {}]]
+#endif
+
+int main () { bo^b (); return 0; }
+)cpp");
+
+  Annotations BarSource(R"cpp(
+#ifdef MACRO
+this is an error
+#endif
+)cpp");
+
+  Annotations BazSource(R"cpp(
+int hello;
+)cpp");
+
+  MockFSProvider FS;
+  MockCompilationDatabase CDB;
+  MultipleErrorCHeckingDiagConsumer DiagConsumer;
+  ClangdServer Server(CDB, DiagConsumer, FS,
+  /*AsyncThreadsCount=*/0,
+  /*StorePreamblesInMemory=*/true);
+
+  auto FooCpp = testPath("foo.cpp");
+  auto BarCpp = testPath("bar.cpp");
+  auto BazCpp = testPath("baz.cpp");
+
+  FS.Files[FooCpp] = "";
+  FS.Files[BarCpp] = "";
+  FS.Files[BazCpp] = "";
+
+  CDB.ExtraClangFlags = {"-DMACRO=1"};
+  Server.addDocument(FooCpp, FooSource.code());
+  Server.addDocument(BarCpp, BarSource.code());
+  Server.addDocument(BazCpp, BazSource.code());
+
+  EXPECT_TRUE(DiagConsumer.contains(FooCpp));
+  EXPECT_TRUE(DiagConsumer.contains(BarCpp));
+  EXPECT_TRUE(DiagConsumer.contains(BazCpp));
+  EXPECT_FALSE(DiagConsumer.lastHadError(FooCpp));
+  EXPECT_TRUE(DiagConsumer.lastHadError(BarCpp));
+  EXPECT_FALSE(DiagConsumer.lastHadError(BazCpp));
+
+  auto Locations = runFindDefinitions(Server, FooCpp, FooSource.point());
+  EXPECT_TRUE(bool(Locations));
+  EXPECT_THAT(Locations->Value, ElementsAre(Location{URIForFile{FooCpp},
+ FooSource.range("one")}));
+
+  // Undefine MACRO, close baz.cpp.
+  CDB.ExtraClangFlags.clear();
+  DiagConsumer.clear();
+  Server.removeDocument(BazCpp);
+  Server.reparseOpenedFiles();
+
+  EXPECT_TRUE(DiagConsumer.contains(FooCpp));
+  EXPECT_TRUE(DiagConsumer.contains(BarCpp));
+  EXPECT_FALSE(DiagConsumer.contains(BazCpp));
+  EXPECT_FALSE(DiagConsumer.lastHadError(FooCpp));
+  EXPECT_FALSE(DiagConsumer.lastHadError(BarCpp));
+
+  Locations = runFindDefinitions(Server, FooCpp, FooSource.point());
+  EXPECT_TRUE(bool(Locations));
+  EXPECT_THAT(Locations->Value, ElementsAre(Location{URIForFile{FooCpp},
+ FooSource.range("two")}));
+}
+
 TEST_F(ClangdVFSTest, MemoryUsage) {
   MockFSProvider FS;
   ErrorCheckingDiagConsumer DiagConsumer;
Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -52,6 +52,7 @@
   virtual void onRename(RenameParams ) = 0;
   virtual void onDocumentHighlight(TextDocumentPositionParams ) = 0;
   virtual void onHover(TextDocumentPositionParams ) = 0;
+  virtual void onChangeConfiguration(DidChangeConfigurationParams ) = 0;
 };
 
 

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

In https://reviews.llvm.org/D39571#1011877, @ilya-biryukov wrote:

> > That looks like another preamble-handling bug. It's much easier to fix and 
> > it's clangd-specific, so I'll make sure to fix that soon.
> >  Thanks for bringing this up, we haven't been testing compile command 
> > changes thoroughly. I'll come up with a fix shortly.
> > 
> > Before this is fixed, you could try writing a similar test with other, 
> > non-preprocessor, flags. See `ClangdVFSTest.ForceReparseCompileCommand` 
> > that tests a similar thing by changing `-xc` to `-xc++`.
> >  I'll make sure to test the `-D` case when fixing the bug.
>
> Should be fixed by https://reviews.llvm.org/D43454.


Thanks, I think I'll simply wait for that fix to get in.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D39571



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

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In https://reviews.llvm.org/D39571#1011842, @ilya-biryukov wrote:

> In https://reviews.llvm.org/D39571#1007393, @simark wrote:
>
> > If I do it in this order, I get one diagnostic both times, when I don't 
> > expect one the second time the file is parsed.  But if I do it the other 
> > way (first parse with no errors, second parse with an error), it works fine.
>
>
> That looks like another preamble-handling bug. It's much easier to fix and 
> it's clangd-specific, so I'll make sure to fix that soon.
>  Thanks for bringing this up, we haven't been testing compile command changes 
> thoroughly. I'll come up with a fix shortly.
>
> Before this is fixed, you could try writing a similar test with other, 
> non-preprocessor, flags. See `ClangdVFSTest.ForceReparseCompileCommand` that 
> tests a similar thing by changing `-xc` to `-xc++`.
>  I'll make sure to test the `-D` case when fixing the bug.


Should be fixed by https://reviews.llvm.org/D43454.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D39571



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

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In https://reviews.llvm.org/D39571#1007393, @simark wrote:

> If I do it in this order, I get one diagnostic both times, when I don't 
> expect one the second time the file is parsed.  But if I do it the other way 
> (first parse with no errors, second parse with an error), it works fine.


That looks like another preamble-handling bug. It's much easier to fix and it's 
clangd-specific, so I'll make sure to fix that soon.
Thanks for bringing this up, we haven't been testing compile command changes 
thoroughly. I'll come up with a fix shortly.

Before this is fixed, you could try writing a similar test with other, 
non-preprocessor, flags. See `ClangdVFSTest.ForceReparseCompileCommand` that 
tests a similar thing by changing `-xc` to `-xc++`.
I'll make sure to test the `-D` case when fixing the bug.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D39571



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

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

In https://reviews.llvm.org/D39571#1007291, @ilya-biryukov wrote:

> It looks like a bug in the preamble handling. (It does not check if macros 
> were redefined).
>  You can workaround that by making sure the preamble ends before your code 
> starts (preamble only captures preprocessor directives, so any C++ decl will 
> end it):
>
> Annotations SourceAnnotations(R"cpp(
>   int avoid_preamble;
>  
>   #ifndef MACRO
>   $before[[static void bob() {}]]
>   #else
>   $after[[static void bob() {}]]
>   #endif
>   /// 
>   )cpp"
>


Ah ok, indeed this test now works when I add this.

The other test I am working on (at the bottom of the file) acts weird, even if 
I add `int avoid_preamble`.  The idea of the test is:

1. Set -DWITH_ERROR=1 in the compile commands database
2. Add the document, expect one error diagnostic
3. Unset WITH_ERROR in the compile commands database
4. Reparse the file, expect no error diagnostic

If I do it in this order, I get one diagnostic both times, when I don't expect 
one the second time the file is parsed.  But if I do it the other way (first 
parse with no errors, second parse with an error), it works fine.




Comment at: unittests/clangd/XRefsTests.cpp:260
 
+TEST(DidChangeConfiguration, DifferentDeclaration) {
+  Annotations SourceAnnotations(R"cpp(

ilya-biryukov wrote:
> I'd move it to `ClangdTests.cpp`, generic `ClangdServer` tests usually go 
> there. It's fine to `#include "Annotations.h"` there, too, even though it 
> hasn't been used before.
> 
Ok.  I put it there for rapid prototyping, but I also though it didn't really 
belong there.



Comment at: unittests/clangd/XRefsTests.cpp:271
+
+  MockCompilationDatabase CDB(/*UseRelPaths=*/true);
+  MockFSProvider FS;

ilya-biryukov wrote:
> Specifying `/*UseRelPath=*/true` is not necessary for this test, default 
> value should do.
Ok.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D39571



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

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In https://reviews.llvm.org/D39571#1006667, @simark wrote:

>   It seems to me like
>
> changes in command line arguments (adding -DMACRO=1) are not taken into 
> account
>  when changing configuration.


It looks like a bug in the preamble handling. (It does not check if macros were 
redefined).
You can workaround that by making sure the preamble ends before your code 
starts (preamble only captures preprocessor directives, so any C++ decl will 
end it):

Annotations SourceAnnotations(R"cpp(
  int avoid_preamble;
  
  #ifndef MACRO
  $before[[static void bob() {}]]
  #else
  $after[[static void bob() {}]]
  #endif
  /// 
  )cpp"






Comment at: unittests/clangd/XRefsTests.cpp:260
 
+TEST(DidChangeConfiguration, DifferentDeclaration) {
+  Annotations SourceAnnotations(R"cpp(

I'd move it to `ClangdTests.cpp`, generic `ClangdServer` tests usually go 
there. It's fine to `#include "Annotations.h"` there, too, even though it 
hasn't been used before.




Comment at: unittests/clangd/XRefsTests.cpp:271
+
+  MockCompilationDatabase CDB(/*UseRelPaths=*/true);
+  MockFSProvider FS;

Specifying `/*UseRelPath=*/true` is not necessary for this test, default value 
should do.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D39571



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

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 134097.
simark added a comment.

Add tests, work in progress

Can you take a look at the "TEST(DidChangeConfiguration, DifferentDeclaration)"
test, and tell me if you see anything wrong with it?  It seems to me like
changes in command line arguments (adding -DMACRO=1) are not taken into account
when changing configuration.

The other test (also very much work-in-progress) would be to verify that
changing configuration gets us the right diagnostics.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D39571

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/DraftStore.cpp
  clangd/DraftStore.h
  clangd/GlobalCompilationDatabase.cpp
  clangd/GlobalCompilationDatabase.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -257,6 +257,86 @@
SourceAnnotations.range()}));
 }
 
+TEST(DidChangeConfiguration, DifferentDeclaration) {
+  Annotations SourceAnnotations(R"cpp(
+#ifndef MACRO
+$before[[static void bob() {}]]
+#else
+$after[[static void bob() {}]]
+#endif
+
+int main() { b^ob(); }
+)cpp");
+
+  MockCompilationDatabase CDB(/*UseRelPaths=*/true);
+  MockFSProvider FS;
+
+  auto FooCpp = getVirtualTestFilePath("foo.cpp");
+  FS.Files[FooCpp] = "";
+
+  IgnoreDiagnostics DiagConsumer;
+  ClangdServer Server(CDB, DiagConsumer, FS, /*AsyncThreadsCount=*/0,
+  /*StorePreambleInMemory=*/true);
+
+  Server.addDocument(FooCpp, SourceAnnotations.code());
+
+  /* Without MACRO defined.  */
+  auto Locations = Server.findDefinitions(FooCpp, SourceAnnotations.point());
+  EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(Locations->Value,
+  ElementsAre(Location{URIForFile{FooCpp.str()},
+   SourceAnnotations.range("before")}));
+
+  CDB.ExtraClangFlags.push_back("-DMACRO=1");
+  Server.reparseOpenedFiles();
+
+  /* With MACRO defined.  */
+  Locations = Server.findDefinitions(FooCpp, SourceAnnotations.point());
+  EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(Locations->Value,
+  ElementsAre(Location{URIForFile{FooCpp.str()},
+   SourceAnnotations.range("after")}));
+}
+
+class MyDiags : public DiagnosticsConsumer {
+  void onDiagnosticsReady(
+  PathRef File, Tagged Diagnostics) override {
+std::cout << "There are " << Diagnostics.Value.size() << std::endl;
+for (DiagWithFixIts  : Diagnostics.Value) {
+  std::cout << "  " << Diag.Diag.message << std::endl;
+}
+  }
+};
+
+TEST(DidChangeConfiguration, Diagnostics) {
+  Annotations SourceAnnotations(R"cpp(
+#ifdef WITH_ERROR
+this is an error
+#endif
+
+int main() { return 0; }
+)cpp");
+
+  MockCompilationDatabase CDB(/*UseRelPaths=*/true);
+  CDB.ExtraClangFlags.push_back("-DWITH_ERROR=1");
+  MockFSProvider FS;
+
+  auto FooCpp = getVirtualTestFilePath("foo.cpp");
+  FS.Files[FooCpp] = "";
+
+  MyDiags DiagConsumer;
+  ClangdServer Server(CDB, DiagConsumer, FS, /*AsyncThreadsCount=*/0,
+  /*StorePreambleInMemory=*/true);
+
+  Server.addDocument(FooCpp, SourceAnnotations.code());
+
+
+  CDB.ExtraClangFlags.clear();
+
+  Server.reparseOpenedFiles();
+
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -51,6 +51,7 @@
   virtual void onCommand(ExecuteCommandParams ) = 0;
   virtual void onRename(RenameParams ) = 0;
   virtual void onDocumentHighlight(TextDocumentPositionParams ) = 0;
+  virtual void onChangeConfiguration(DidChangeConfigurationParams ) = 0;
 };
 
 void registerCallbackHandlers(JSONRPCDispatcher , JSONOutput ,
Index: clangd/ProtocolHandlers.cpp
===
--- clangd/ProtocolHandlers.cpp
+++ clangd/ProtocolHandlers.cpp
@@ -71,4 +71,6 @@
   Register("workspace/executeCommand", ::onCommand);
   Register("textDocument/documentHighlight",
::onDocumentHighlight);
+  Register("workspace/didChangeConfiguration",
+   ::onChangeConfiguration);
 }
Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -265,6 +265,20 @@
 };
 bool fromJSON(const json::Expr &, DidChangeWatchedFilesParams &);
 
+/// Clangd extension to manage a workspace/didChangeConfiguration notification
+/// since the data received is described as 'any' type in LSP.
+struct ClangdConfigurationParamsChange {
+  

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

I think I managed to make some tests by using the `MockCompilationDatabase`.  
Basically with some code like:

  #ifndef MACRO
  static void func () {}  // 1
  #else
  static void func () {}  // 2
  #endif

and these steps:

1. Server.addDocument(...)
2. Server.findDefinitions (assert that it returns definition 1)
3. CDB.ExtraClangFlags.push_back("-DMACRO=1")
4. Server.reparseOpenedFiles()
5. Server.findDefinitions (assert that it returns definition 2)

Right now that test fails, but it's not clear to me whether it's because the 
test is wrong or there's really a bug in there.  I'll upload a work-in-progress 
version.




Comment at: clangd/ClangdServer.cpp:541
 
+std::vector
+ClangdServer::reparseOpenedFiles() {

ilya-biryukov wrote:
> We're not returning futures from `forceReparse` anymore, this function has to 
> be updated accordingly.
Indeed, I found this by rebasing.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D39571



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

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In https://reviews.llvm.org/D39571#1005926, @malaperle wrote:

> I haven't looked at the newest patch yet but I gave it a quick try and saw 
> something odd. If I change the configuration to something invalid (say I 
> specify the path to a CMakeLists.txt), then I get many errors/diagnostics, 
> which is normal. But then when I change the config to something valid, the 
> same diagnostics re-emitted, as if the configuration failed to change. I'll 
> check the code tomorrow a bit but I thought I'd share this with you early.


Maybe you were still seeing diagnostics from the older versions of the files? 
clangd should **eventually** produce the right diagnostics, but if processing 
of some of the requests is in-flight at the time there results may still be 
reported.
BTW, I don't know if you've seen it before, but there's `-input-mirror-file` 
option to clangd that records the lsp input which can be used to rerun clangd 
later.

  # This should be run by VSCode (or Theia)
  clangd -input-mirror-file=/tmp/clangd.input
  
  # Later we can rerun from command line
  $ clangd < /tmp/clangd.input
  # Pass the flag to execute everyhing on one thread
  $ clangd -run-synchronously < /tmp/clangd.input

This is useful to debug/rerun and see the logs, etc.

This patch is good to go after remove `std::future<>`  from 
`reparseOpenedFiles`, unless @malaperle discovers there are other problems. 
Ideally, it'd be also nice to have a test for it.




Comment at: clangd/ClangdServer.cpp:541
 
+std::vector
+ClangdServer::reparseOpenedFiles() {

We're not returning futures from `forceReparse` anymore, this function has to 
be updated accordingly.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D39571



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

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

I haven't looked at the newest patch yet but I gave it a quick try and saw 
something odd. If I change the configuration to something invalid (say I 
specify the path to a CMakeLists.txt), then I get many errors/diagnostics, 
which is normal. But then when I change the config to something valid, the same 
diagnostics re-emitted, as if the configuration failed to change. I'll check 
the code tomorrow a bit but I thought I'd share this with you early.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D39571



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

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 133034.
simark added a comment.

Fix assertion about parsing a document that is not open

As found by Ilya, the getActiveFiles method would return the documents that 
were previously opened and then closed.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D39571

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/DraftStore.cpp
  clangd/DraftStore.h
  clangd/GlobalCompilationDatabase.cpp
  clangd/GlobalCompilationDatabase.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h

Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -51,6 +51,7 @@
   virtual void onCommand(ExecuteCommandParams ) = 0;
   virtual void onRename(RenameParams ) = 0;
   virtual void onDocumentHighlight(TextDocumentPositionParams ) = 0;
+  virtual void onChangeConfiguration(DidChangeConfigurationParams ) = 0;
 };
 
 void registerCallbackHandlers(JSONRPCDispatcher , JSONOutput ,
Index: clangd/ProtocolHandlers.cpp
===
--- clangd/ProtocolHandlers.cpp
+++ clangd/ProtocolHandlers.cpp
@@ -71,4 +71,6 @@
   Register("workspace/executeCommand", ::onCommand);
   Register("textDocument/documentHighlight",
::onDocumentHighlight);
+  Register("workspace/didChangeConfiguration",
+   ::onChangeConfiguration);
 }
Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -265,6 +265,20 @@
 };
 bool fromJSON(const json::Expr &, DidChangeWatchedFilesParams &);
 
+/// Clangd extension to manage a workspace/didChangeConfiguration notification
+/// since the data received is described as 'any' type in LSP.
+struct ClangdConfigurationParamsChange {
+  llvm::Optional compilationDatabasePath;
+};
+bool fromJSON(const json::Expr &, ClangdConfigurationParamsChange &);
+
+struct DidChangeConfigurationParams {
+  // We use this predefined struct because it is easier to use
+  // than the protocol specified type of 'any'.
+  ClangdConfigurationParamsChange settings;
+};
+bool fromJSON(const json::Expr &, DidChangeConfigurationParams &);
+
 struct FormattingOptions {
   /// Size of a tab in spaces.
   int tabSize;
Index: clangd/Protocol.cpp
===
--- clangd/Protocol.cpp
+++ clangd/Protocol.cpp
@@ -400,5 +400,15 @@
   };
 }
 
+bool fromJSON(const json::Expr , DidChangeConfigurationParams ) {
+  json::ObjectMapper O(Params);
+  return O && O.map("settings", CCP.settings);
+}
+
+bool fromJSON(const json::Expr , ClangdConfigurationParamsChange ) {
+  json::ObjectMapper O(Params);
+  return O && O.map("compilationDatabasePath", CCPC.compilationDatabasePath);
+}
+
 } // namespace clangd
 } // namespace clang
Index: clangd/GlobalCompilationDatabase.h
===
--- clangd/GlobalCompilationDatabase.h
+++ clangd/GlobalCompilationDatabase.h
@@ -61,6 +61,9 @@
   /// Uses the default fallback command, adding any extra flags.
   tooling::CompileCommand getFallbackCommand(PathRef File) const override;
 
+  /// Set the compile commands directory to \p P.
+  void setCompileCommandsDir(Path P);
+
   /// Sets the extra flags that should be added to a file.
   void setExtraFlagsForFile(PathRef File, std::vector ExtraFlags);
 
Index: clangd/GlobalCompilationDatabase.cpp
===
--- clangd/GlobalCompilationDatabase.cpp
+++ clangd/GlobalCompilationDatabase.cpp
@@ -51,6 +51,12 @@
   return C;
 }
 
+void DirectoryBasedGlobalCompilationDatabase::setCompileCommandsDir(Path P) {
+  std::lock_guard Lock(Mutex);
+  CompileCommandsDir = P;
+  CompilationDatabases.clear();
+}
+
 void DirectoryBasedGlobalCompilationDatabase::setExtraFlagsForFile(
 PathRef File, std::vector ExtraFlags) {
   std::lock_guard Lock(Mutex);
Index: clangd/DraftStore.h
===
--- clangd/DraftStore.h
+++ clangd/DraftStore.h
@@ -40,6 +40,12 @@
   /// \return version and contents of the stored document.
   /// For untracked files, a (0, None) pair is returned.
   VersionedDraft getDraft(PathRef File) const;
+
+  /// \return List of names of active drafts in this store.  Drafts that were
+  /// removed (which still have an entry in the Drafts map) are not returned by
+  /// this function.
+  std::vector getActiveFiles() const;
+
   /// \return version of the tracked document.
   /// For untracked files, 0 is returned.
   DocVersion getVersion(PathRef File) const;
Index: clangd/DraftStore.cpp
===
--- clangd/DraftStore.cpp
+++ clangd/DraftStore.cpp
@@ -21,6 +21,17 @@
   return 

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/ClangdLSPServer.cpp:302
 
+// FIXME: This function needs to be properly tested.
+void ClangdLSPServer::onChangeConfiguration(

simark wrote:
> ilya-biryukov wrote:
> > simark wrote:
> > > simark wrote:
> > > > ilya-biryukov wrote:
> > > > > simark wrote:
> > > > > > simark wrote:
> > > > > > > ilya-biryukov wrote:
> > > > > > > > simark wrote:
> > > > > > > > > ilya-biryukov wrote:
> > > > > > > > > > simark wrote:
> > > > > > > > > > > ilya-biryukov wrote:
> > > > > > > > > > > > Are you planning to to address this FIXME before 
> > > > > > > > > > > > checking the code in?
> > > > > > > > > > > Following what you said here:
> > > > > > > > > > > 
> > > > > > > > > > > https://reviews.llvm.org/D39571?id=124024#inline-359345
> > > > > > > > > > > 
> > > > > > > > > > > I have not really looked into what was wrong with the 
> > > > > > > > > > > test, and what is missing in the infrastructure to make 
> > > > > > > > > > > it work.  But I assumed that the situation did not change 
> > > > > > > > > > > since then.  Can you enlighten me on what the problem 
> > > > > > > > > > > was, and what is missing?
> > > > > > > > > > We usually write unittests for that kind of thing, since 
> > > > > > > > > > they allow to plug an in-memory filesystem, but we only 
> > > > > > > > > > test `ClangdServer` (examples are in 
> > > > > > > > > > `unittests/clangd/ClangdTests.cpp`). `ClangdLSPServer` does 
> > > > > > > > > > not allow to plug in a virtual filesystem (vfs). Even if we 
> > > > > > > > > > add vfs, it's still hard to unit-test because we'll have to 
> > > > > > > > > > match the json input/output directly.
> > > > > > > > > > 
> > > > > > > > > > This leaves us with an option of a lit test that runs 
> > > > > > > > > > `clangd` directly, similar to tests in `test/clangd`.
> > > > > > > > > > The lit test would need to create a temporary directory, 
> > > > > > > > > > create proper `compile_commands.json` there, then send the 
> > > > > > > > > > LSP commands with the path to the test to clangd.
> > > > > > > > > > One major complication is that in LSP we have to specify 
> > > > > > > > > > the size of each message, but in our case the size would 
> > > > > > > > > > change depending on created temp path. It means we'll have 
> > > > > > > > > > to patch the test input to setup proper paths and message 
> > > > > > > > > > sizes.
> > > > > > > > > > If we choose to go down this path, 
> > > > > > > > > > `clang-tools-extra/test/clang-tidy/vfsoverlay.cpp` does a 
> > > > > > > > > > similar setup (create temp-dir, patch up some configuration 
> > > > > > > > > > files to point into the temp directory, etc) and could be 
> > > > > > > > > > used as a starting point.
> > > > > > > > > > 
> > > > > > > > > > It's not impossible to write that test, it's just a bit 
> > > > > > > > > > involved. Having a test would be nice, though, to ensure we 
> > > > > > > > > > don't break this method while doing other things. 
> > > > > > > > > > Especially given that this functionality is not used 
> > > > > > > > > > anywhere in clangd.
> > > > > > > > > > We usually write unittests for that kind of thing, since 
> > > > > > > > > > they allow to plug an in-memory filesystem, but we only 
> > > > > > > > > > test ClangdServer (examples are in 
> > > > > > > > > > unittests/clangd/ClangdTests.cpp). ClangdLSPServer does not 
> > > > > > > > > > allow to plug in a virtual filesystem (vfs). Even if we add 
> > > > > > > > > > vfs, it's still hard to unit-test because we'll have to 
> > > > > > > > > > match the json input/output directly.
> > > > > > > > > 
> > > > > > > > > What do you mean by "we'll have to match the json 
> > > > > > > > > input/output directly"?  That we'll have to match the 
> > > > > > > > > complete JSON output textually?  Couldn't the test parse the 
> > > > > > > > > JSON into some data structures, then we could assert specific 
> > > > > > > > > things, like that this particular field is present and 
> > > > > > > > > contains a certain substring, for example?
> > > > > > > > > 
> > > > > > > > > > This leaves us with an option of a lit test that runs 
> > > > > > > > > > clangd directly, similar to tests in test/clangd.
> > > > > > > > > > The lit test would need to create a temporary directory, 
> > > > > > > > > > create proper compile_commands.json there, then send the 
> > > > > > > > > > LSP commands with the path to the test to clangd.
> > > > > > > > > > One major complication is that in LSP we have to specify 
> > > > > > > > > > the size of each message, but in our case the size would 
> > > > > > > > > > change depending on created temp path. It means we'll have 
> > > > > > > > > > to patch the test input to setup proper paths and message 
> > > > > > > > > > sizes.
> > > > > > > > > > If we choose to go down this path, 
> > > > > > > > > > clang-tools-extra/test/clang-tidy/vfsoverlay.cpp does a 

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Simon Marchi via Phabricator via cfe-commits]

simark added inline comments.



Comment at: clangd/ClangdLSPServer.cpp:302
 
+// FIXME: This function needs to be properly tested.
+void ClangdLSPServer::onChangeConfiguration(

ilya-biryukov wrote:
> simark wrote:
> > simark wrote:
> > > ilya-biryukov wrote:
> > > > simark wrote:
> > > > > simark wrote:
> > > > > > ilya-biryukov wrote:
> > > > > > > simark wrote:
> > > > > > > > ilya-biryukov wrote:
> > > > > > > > > simark wrote:
> > > > > > > > > > ilya-biryukov wrote:
> > > > > > > > > > > Are you planning to to address this FIXME before checking 
> > > > > > > > > > > the code in?
> > > > > > > > > > Following what you said here:
> > > > > > > > > > 
> > > > > > > > > > https://reviews.llvm.org/D39571?id=124024#inline-359345
> > > > > > > > > > 
> > > > > > > > > > I have not really looked into what was wrong with the test, 
> > > > > > > > > > and what is missing in the infrastructure to make it work.  
> > > > > > > > > > But I assumed that the situation did not change since then. 
> > > > > > > > > >  Can you enlighten me on what the problem was, and what is 
> > > > > > > > > > missing?
> > > > > > > > > We usually write unittests for that kind of thing, since they 
> > > > > > > > > allow to plug an in-memory filesystem, but we only test 
> > > > > > > > > `ClangdServer` (examples are in 
> > > > > > > > > `unittests/clangd/ClangdTests.cpp`). `ClangdLSPServer` does 
> > > > > > > > > not allow to plug in a virtual filesystem (vfs). Even if we 
> > > > > > > > > add vfs, it's still hard to unit-test because we'll have to 
> > > > > > > > > match the json input/output directly.
> > > > > > > > > 
> > > > > > > > > This leaves us with an option of a lit test that runs 
> > > > > > > > > `clangd` directly, similar to tests in `test/clangd`.
> > > > > > > > > The lit test would need to create a temporary directory, 
> > > > > > > > > create proper `compile_commands.json` there, then send the 
> > > > > > > > > LSP commands with the path to the test to clangd.
> > > > > > > > > One major complication is that in LSP we have to specify the 
> > > > > > > > > size of each message, but in our case the size would change 
> > > > > > > > > depending on created temp path. It means we'll have to patch 
> > > > > > > > > the test input to setup proper paths and message sizes.
> > > > > > > > > If we choose to go down this path, 
> > > > > > > > > `clang-tools-extra/test/clang-tidy/vfsoverlay.cpp` does a 
> > > > > > > > > similar setup (create temp-dir, patch up some configuration 
> > > > > > > > > files to point into the temp directory, etc) and could be 
> > > > > > > > > used as a starting point.
> > > > > > > > > 
> > > > > > > > > It's not impossible to write that test, it's just a bit 
> > > > > > > > > involved. Having a test would be nice, though, to ensure we 
> > > > > > > > > don't break this method while doing other things. Especially 
> > > > > > > > > given that this functionality is not used anywhere in clangd.
> > > > > > > > > We usually write unittests for that kind of thing, since they 
> > > > > > > > > allow to plug an in-memory filesystem, but we only test 
> > > > > > > > > ClangdServer (examples are in 
> > > > > > > > > unittests/clangd/ClangdTests.cpp). ClangdLSPServer does not 
> > > > > > > > > allow to plug in a virtual filesystem (vfs). Even if we add 
> > > > > > > > > vfs, it's still hard to unit-test because we'll have to match 
> > > > > > > > > the json input/output directly.
> > > > > > > > 
> > > > > > > > What do you mean by "we'll have to match the json input/output 
> > > > > > > > directly"?  That we'll have to match the complete JSON output 
> > > > > > > > textually?  Couldn't the test parse the JSON into some data 
> > > > > > > > structures, then we could assert specific things, like that 
> > > > > > > > this particular field is present and contains a certain 
> > > > > > > > substring, for example?
> > > > > > > > 
> > > > > > > > > This leaves us with an option of a lit test that runs clangd 
> > > > > > > > > directly, similar to tests in test/clangd.
> > > > > > > > > The lit test would need to create a temporary directory, 
> > > > > > > > > create proper compile_commands.json there, then send the LSP 
> > > > > > > > > commands with the path to the test to clangd.
> > > > > > > > > One major complication is that in LSP we have to specify the 
> > > > > > > > > size of each message, but in our case the size would change 
> > > > > > > > > depending on created temp path. It means we'll have to patch 
> > > > > > > > > the test input to setup proper paths and message sizes.
> > > > > > > > > If we choose to go down this path, 
> > > > > > > > > clang-tools-extra/test/clang-tidy/vfsoverlay.cpp does a 
> > > > > > > > > similar setup (create temp-dir, patch up some configuration 
> > > > > > > > > files to point into the temp directory, etc) and could be 
> > > > > > > > > used as a starting point.
> > > > > > > > 
> > > > > > > > 

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/ClangdLSPServer.cpp:302
 
+// FIXME: This function needs to be properly tested.
+void ClangdLSPServer::onChangeConfiguration(

simark wrote:
> simark wrote:
> > ilya-biryukov wrote:
> > > simark wrote:
> > > > simark wrote:
> > > > > ilya-biryukov wrote:
> > > > > > simark wrote:
> > > > > > > ilya-biryukov wrote:
> > > > > > > > simark wrote:
> > > > > > > > > ilya-biryukov wrote:
> > > > > > > > > > Are you planning to to address this FIXME before checking 
> > > > > > > > > > the code in?
> > > > > > > > > Following what you said here:
> > > > > > > > > 
> > > > > > > > > https://reviews.llvm.org/D39571?id=124024#inline-359345
> > > > > > > > > 
> > > > > > > > > I have not really looked into what was wrong with the test, 
> > > > > > > > > and what is missing in the infrastructure to make it work.  
> > > > > > > > > But I assumed that the situation did not change since then.  
> > > > > > > > > Can you enlighten me on what the problem was, and what is 
> > > > > > > > > missing?
> > > > > > > > We usually write unittests for that kind of thing, since they 
> > > > > > > > allow to plug an in-memory filesystem, but we only test 
> > > > > > > > `ClangdServer` (examples are in 
> > > > > > > > `unittests/clangd/ClangdTests.cpp`). `ClangdLSPServer` does not 
> > > > > > > > allow to plug in a virtual filesystem (vfs). Even if we add 
> > > > > > > > vfs, it's still hard to unit-test because we'll have to match 
> > > > > > > > the json input/output directly.
> > > > > > > > 
> > > > > > > > This leaves us with an option of a lit test that runs `clangd` 
> > > > > > > > directly, similar to tests in `test/clangd`.
> > > > > > > > The lit test would need to create a temporary directory, create 
> > > > > > > > proper `compile_commands.json` there, then send the LSP 
> > > > > > > > commands with the path to the test to clangd.
> > > > > > > > One major complication is that in LSP we have to specify the 
> > > > > > > > size of each message, but in our case the size would change 
> > > > > > > > depending on created temp path. It means we'll have to patch 
> > > > > > > > the test input to setup proper paths and message sizes.
> > > > > > > > If we choose to go down this path, 
> > > > > > > > `clang-tools-extra/test/clang-tidy/vfsoverlay.cpp` does a 
> > > > > > > > similar setup (create temp-dir, patch up some configuration 
> > > > > > > > files to point into the temp directory, etc) and could be used 
> > > > > > > > as a starting point.
> > > > > > > > 
> > > > > > > > It's not impossible to write that test, it's just a bit 
> > > > > > > > involved. Having a test would be nice, though, to ensure we 
> > > > > > > > don't break this method while doing other things. Especially 
> > > > > > > > given that this functionality is not used anywhere in clangd.
> > > > > > > > We usually write unittests for that kind of thing, since they 
> > > > > > > > allow to plug an in-memory filesystem, but we only test 
> > > > > > > > ClangdServer (examples are in 
> > > > > > > > unittests/clangd/ClangdTests.cpp). ClangdLSPServer does not 
> > > > > > > > allow to plug in a virtual filesystem (vfs). Even if we add 
> > > > > > > > vfs, it's still hard to unit-test because we'll have to match 
> > > > > > > > the json input/output directly.
> > > > > > > 
> > > > > > > What do you mean by "we'll have to match the json input/output 
> > > > > > > directly"?  That we'll have to match the complete JSON output 
> > > > > > > textually?  Couldn't the test parse the JSON into some data 
> > > > > > > structures, then we could assert specific things, like that this 
> > > > > > > particular field is present and contains a certain substring, for 
> > > > > > > example?
> > > > > > > 
> > > > > > > > This leaves us with an option of a lit test that runs clangd 
> > > > > > > > directly, similar to tests in test/clangd.
> > > > > > > > The lit test would need to create a temporary directory, create 
> > > > > > > > proper compile_commands.json there, then send the LSP commands 
> > > > > > > > with the path to the test to clangd.
> > > > > > > > One major complication is that in LSP we have to specify the 
> > > > > > > > size of each message, but in our case the size would change 
> > > > > > > > depending on created temp path. It means we'll have to patch 
> > > > > > > > the test input to setup proper paths and message sizes.
> > > > > > > > If we choose to go down this path, 
> > > > > > > > clang-tools-extra/test/clang-tidy/vfsoverlay.cpp does a similar 
> > > > > > > > setup (create temp-dir, patch up some configuration files to 
> > > > > > > > point into the temp directory, etc) and could be used as a 
> > > > > > > > starting point.
> > > > > > > 
> > > > > > > Ok, I see the complication with the Content-Length.  I am not 
> > > > > > > familiar with lit yet, so I don't know what it is capable of.  
> > > > > > > But being able 

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Simon Marchi via Phabricator via cfe-commits]

simark added inline comments.



Comment at: clangd/ClangdLSPServer.cpp:302
 
+// FIXME: This function needs to be properly tested.
+void ClangdLSPServer::onChangeConfiguration(

simark wrote:
> ilya-biryukov wrote:
> > simark wrote:
> > > simark wrote:
> > > > ilya-biryukov wrote:
> > > > > simark wrote:
> > > > > > ilya-biryukov wrote:
> > > > > > > simark wrote:
> > > > > > > > ilya-biryukov wrote:
> > > > > > > > > Are you planning to to address this FIXME before checking the 
> > > > > > > > > code in?
> > > > > > > > Following what you said here:
> > > > > > > > 
> > > > > > > > https://reviews.llvm.org/D39571?id=124024#inline-359345
> > > > > > > > 
> > > > > > > > I have not really looked into what was wrong with the test, and 
> > > > > > > > what is missing in the infrastructure to make it work.  But I 
> > > > > > > > assumed that the situation did not change since then.  Can you 
> > > > > > > > enlighten me on what the problem was, and what is missing?
> > > > > > > We usually write unittests for that kind of thing, since they 
> > > > > > > allow to plug an in-memory filesystem, but we only test 
> > > > > > > `ClangdServer` (examples are in 
> > > > > > > `unittests/clangd/ClangdTests.cpp`). `ClangdLSPServer` does not 
> > > > > > > allow to plug in a virtual filesystem (vfs). Even if we add vfs, 
> > > > > > > it's still hard to unit-test because we'll have to match the json 
> > > > > > > input/output directly.
> > > > > > > 
> > > > > > > This leaves us with an option of a lit test that runs `clangd` 
> > > > > > > directly, similar to tests in `test/clangd`.
> > > > > > > The lit test would need to create a temporary directory, create 
> > > > > > > proper `compile_commands.json` there, then send the LSP commands 
> > > > > > > with the path to the test to clangd.
> > > > > > > One major complication is that in LSP we have to specify the size 
> > > > > > > of each message, but in our case the size would change depending 
> > > > > > > on created temp path. It means we'll have to patch the test input 
> > > > > > > to setup proper paths and message sizes.
> > > > > > > If we choose to go down this path, 
> > > > > > > `clang-tools-extra/test/clang-tidy/vfsoverlay.cpp` does a similar 
> > > > > > > setup (create temp-dir, patch up some configuration files to 
> > > > > > > point into the temp directory, etc) and could be used as a 
> > > > > > > starting point.
> > > > > > > 
> > > > > > > It's not impossible to write that test, it's just a bit involved. 
> > > > > > > Having a test would be nice, though, to ensure we don't break 
> > > > > > > this method while doing other things. Especially given that this 
> > > > > > > functionality is not used anywhere in clangd.
> > > > > > > We usually write unittests for that kind of thing, since they 
> > > > > > > allow to plug an in-memory filesystem, but we only test 
> > > > > > > ClangdServer (examples are in unittests/clangd/ClangdTests.cpp). 
> > > > > > > ClangdLSPServer does not allow to plug in a virtual filesystem 
> > > > > > > (vfs). Even if we add vfs, it's still hard to unit-test because 
> > > > > > > we'll have to match the json input/output directly.
> > > > > > 
> > > > > > What do you mean by "we'll have to match the json input/output 
> > > > > > directly"?  That we'll have to match the complete JSON output 
> > > > > > textually?  Couldn't the test parse the JSON into some data 
> > > > > > structures, then we could assert specific things, like that this 
> > > > > > particular field is present and contains a certain substring, for 
> > > > > > example?
> > > > > > 
> > > > > > > This leaves us with an option of a lit test that runs clangd 
> > > > > > > directly, similar to tests in test/clangd.
> > > > > > > The lit test would need to create a temporary directory, create 
> > > > > > > proper compile_commands.json there, then send the LSP commands 
> > > > > > > with the path to the test to clangd.
> > > > > > > One major complication is that in LSP we have to specify the size 
> > > > > > > of each message, but in our case the size would change depending 
> > > > > > > on created temp path. It means we'll have to patch the test input 
> > > > > > > to setup proper paths and message sizes.
> > > > > > > If we choose to go down this path, 
> > > > > > > clang-tools-extra/test/clang-tidy/vfsoverlay.cpp does a similar 
> > > > > > > setup (create temp-dir, patch up some configuration files to 
> > > > > > > point into the temp directory, etc) and could be used as a 
> > > > > > > starting point.
> > > > > > 
> > > > > > Ok, I see the complication with the Content-Length.  I am not 
> > > > > > familiar with lit yet, so I don't know what it is capable of.  But 
> > > > > > being able to craft and send arbitrary LSP messages would certainly 
> > > > > > be helpful in the future for all kinds of black box test, so having 
> > > > > > a framework that allows to do this would be helpful, I think.  

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Simon Marchi via Phabricator via cfe-commits]

simark added inline comments.



Comment at: clangd/ClangdLSPServer.cpp:302
 
+// FIXME: This function needs to be properly tested.
+void ClangdLSPServer::onChangeConfiguration(

ilya-biryukov wrote:
> simark wrote:
> > simark wrote:
> > > ilya-biryukov wrote:
> > > > simark wrote:
> > > > > ilya-biryukov wrote:
> > > > > > simark wrote:
> > > > > > > ilya-biryukov wrote:
> > > > > > > > Are you planning to to address this FIXME before checking the 
> > > > > > > > code in?
> > > > > > > Following what you said here:
> > > > > > > 
> > > > > > > https://reviews.llvm.org/D39571?id=124024#inline-359345
> > > > > > > 
> > > > > > > I have not really looked into what was wrong with the test, and 
> > > > > > > what is missing in the infrastructure to make it work.  But I 
> > > > > > > assumed that the situation did not change since then.  Can you 
> > > > > > > enlighten me on what the problem was, and what is missing?
> > > > > > We usually write unittests for that kind of thing, since they allow 
> > > > > > to plug an in-memory filesystem, but we only test `ClangdServer` 
> > > > > > (examples are in `unittests/clangd/ClangdTests.cpp`). 
> > > > > > `ClangdLSPServer` does not allow to plug in a virtual filesystem 
> > > > > > (vfs). Even if we add vfs, it's still hard to unit-test because 
> > > > > > we'll have to match the json input/output directly.
> > > > > > 
> > > > > > This leaves us with an option of a lit test that runs `clangd` 
> > > > > > directly, similar to tests in `test/clangd`.
> > > > > > The lit test would need to create a temporary directory, create 
> > > > > > proper `compile_commands.json` there, then send the LSP commands 
> > > > > > with the path to the test to clangd.
> > > > > > One major complication is that in LSP we have to specify the size 
> > > > > > of each message, but in our case the size would change depending on 
> > > > > > created temp path. It means we'll have to patch the test input to 
> > > > > > setup proper paths and message sizes.
> > > > > > If we choose to go down this path, 
> > > > > > `clang-tools-extra/test/clang-tidy/vfsoverlay.cpp` does a similar 
> > > > > > setup (create temp-dir, patch up some configuration files to point 
> > > > > > into the temp directory, etc) and could be used as a starting point.
> > > > > > 
> > > > > > It's not impossible to write that test, it's just a bit involved. 
> > > > > > Having a test would be nice, though, to ensure we don't break this 
> > > > > > method while doing other things. Especially given that this 
> > > > > > functionality is not used anywhere in clangd.
> > > > > > We usually write unittests for that kind of thing, since they allow 
> > > > > > to plug an in-memory filesystem, but we only test ClangdServer 
> > > > > > (examples are in unittests/clangd/ClangdTests.cpp). ClangdLSPServer 
> > > > > > does not allow to plug in a virtual filesystem (vfs). Even if we 
> > > > > > add vfs, it's still hard to unit-test because we'll have to match 
> > > > > > the json input/output directly.
> > > > > 
> > > > > What do you mean by "we'll have to match the json input/output 
> > > > > directly"?  That we'll have to match the complete JSON output 
> > > > > textually?  Couldn't the test parse the JSON into some data 
> > > > > structures, then we could assert specific things, like that this 
> > > > > particular field is present and contains a certain substring, for 
> > > > > example?
> > > > > 
> > > > > > This leaves us with an option of a lit test that runs clangd 
> > > > > > directly, similar to tests in test/clangd.
> > > > > > The lit test would need to create a temporary directory, create 
> > > > > > proper compile_commands.json there, then send the LSP commands with 
> > > > > > the path to the test to clangd.
> > > > > > One major complication is that in LSP we have to specify the size 
> > > > > > of each message, but in our case the size would change depending on 
> > > > > > created temp path. It means we'll have to patch the test input to 
> > > > > > setup proper paths and message sizes.
> > > > > > If we choose to go down this path, 
> > > > > > clang-tools-extra/test/clang-tidy/vfsoverlay.cpp does a similar 
> > > > > > setup (create temp-dir, patch up some configuration files to point 
> > > > > > into the temp directory, etc) and could be used as a starting point.
> > > > > 
> > > > > Ok, I see the complication with the Content-Length.  I am not 
> > > > > familiar with lit yet, so I don't know what it is capable of.  But 
> > > > > being able to craft and send arbitrary LSP messages would certainly 
> > > > > be helpful in the future for all kinds of black box test, so having a 
> > > > > framework that allows to do this would be helpful, I think.  I'm not 
> > > > > familiar enough with the ecosystem to do this right now, but I'll 
> > > > > keep it in mind.
> > > > > 
> > > > > One question about this particular test.  Would there be some race 
> > > > > 

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/ClangdLSPServer.cpp:78
 {"documentHighlightProvider", true},
+{"configurationChangeProvider", true},
 {"renameProvider", true},

simark wrote:
> ilya-biryukov wrote:
> > simark wrote:
> > > Nebiroth wrote:
> > > > simark wrote:
> > > > > I find `configurationChangeProvider` a bit weird.  It makes it sound 
> > > > > like clangd can provide configuration changes.  In reality, it can 
> > > > > accept configuration changes.  So I think this should be named 
> > > > > something else.
> > > > Agreed, perhaps configurationChangeManager would be more appropriate 
> > > > then?
> > > I'm thinking of removing it for the time being.  Since it's not defined 
> > > in the protocol what types of configuration changes exist (it's specific 
> > > to each language server), it not very useful to simply advertise that we 
> > > support configuration changes.  We would need to advertise that we 
> > > support compilation database changes in particular.  I think this can be 
> > > done later.
> > `"configurationChangeProvider"` is not in the LSP, right?
> > There's `experimental` field in the specification, let's put it under that 
> > field if you want to advertise that clangd supports this spec to your 
> > clients.
> I've removed it for now, we can add it later.  We should think about how to 
> express what we support.  It's not enough to say we support the 
> `didChangeConfiguration` notification, we should also express what element of 
> configuration we support (the location of the compile commands directory, in 
> this case).
SG. Let's not add extra stuff not in the LSP if the clients can live without it.



Comment at: clangd/ClangdLSPServer.cpp:302
 
+// FIXME: This function needs to be properly tested.
+void ClangdLSPServer::onChangeConfiguration(

simark wrote:
> simark wrote:
> > ilya-biryukov wrote:
> > > simark wrote:
> > > > ilya-biryukov wrote:
> > > > > simark wrote:
> > > > > > ilya-biryukov wrote:
> > > > > > > Are you planning to to address this FIXME before checking the 
> > > > > > > code in?
> > > > > > Following what you said here:
> > > > > > 
> > > > > > https://reviews.llvm.org/D39571?id=124024#inline-359345
> > > > > > 
> > > > > > I have not really looked into what was wrong with the test, and 
> > > > > > what is missing in the infrastructure to make it work.  But I 
> > > > > > assumed that the situation did not change since then.  Can you 
> > > > > > enlighten me on what the problem was, and what is missing?
> > > > > We usually write unittests for that kind of thing, since they allow 
> > > > > to plug an in-memory filesystem, but we only test `ClangdServer` 
> > > > > (examples are in `unittests/clangd/ClangdTests.cpp`). 
> > > > > `ClangdLSPServer` does not allow to plug in a virtual filesystem 
> > > > > (vfs). Even if we add vfs, it's still hard to unit-test because we'll 
> > > > > have to match the json input/output directly.
> > > > > 
> > > > > This leaves us with an option of a lit test that runs `clangd` 
> > > > > directly, similar to tests in `test/clangd`.
> > > > > The lit test would need to create a temporary directory, create 
> > > > > proper `compile_commands.json` there, then send the LSP commands with 
> > > > > the path to the test to clangd.
> > > > > One major complication is that in LSP we have to specify the size of 
> > > > > each message, but in our case the size would change depending on 
> > > > > created temp path. It means we'll have to patch the test input to 
> > > > > setup proper paths and message sizes.
> > > > > If we choose to go down this path, 
> > > > > `clang-tools-extra/test/clang-tidy/vfsoverlay.cpp` does a similar 
> > > > > setup (create temp-dir, patch up some configuration files to point 
> > > > > into the temp directory, etc) and could be used as a starting point.
> > > > > 
> > > > > It's not impossible to write that test, it's just a bit involved. 
> > > > > Having a test would be nice, though, to ensure we don't break this 
> > > > > method while doing other things. Especially given that this 
> > > > > functionality is not used anywhere in clangd.
> > > > > We usually write unittests for that kind of thing, since they allow 
> > > > > to plug an in-memory filesystem, but we only test ClangdServer 
> > > > > (examples are in unittests/clangd/ClangdTests.cpp). ClangdLSPServer 
> > > > > does not allow to plug in a virtual filesystem (vfs). Even if we add 
> > > > > vfs, it's still hard to unit-test because we'll have to match the 
> > > > > json input/output directly.
> > > > 
> > > > What do you mean by "we'll have to match the json input/output 
> > > > directly"?  That we'll have to match the complete JSON output 
> > > > textually?  Couldn't the test parse the JSON into some data structures, 
> > > > then we could assert specific things, like that this particular field 
> > > > 

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Simon Marchi via Phabricator via cfe-commits]

simark added inline comments.



Comment at: clangd/ClangdLSPServer.cpp:78
 {"documentHighlightProvider", true},
+{"configurationChangeProvider", true},
 {"renameProvider", true},

ilya-biryukov wrote:
> simark wrote:
> > Nebiroth wrote:
> > > simark wrote:
> > > > I find `configurationChangeProvider` a bit weird.  It makes it sound 
> > > > like clangd can provide configuration changes.  In reality, it can 
> > > > accept configuration changes.  So I think this should be named 
> > > > something else.
> > > Agreed, perhaps configurationChangeManager would be more appropriate then?
> > I'm thinking of removing it for the time being.  Since it's not defined in 
> > the protocol what types of configuration changes exist (it's specific to 
> > each language server), it not very useful to simply advertise that we 
> > support configuration changes.  We would need to advertise that we support 
> > compilation database changes in particular.  I think this can be done later.
> `"configurationChangeProvider"` is not in the LSP, right?
> There's `experimental` field in the specification, let's put it under that 
> field if you want to advertise that clangd supports this spec to your clients.
I've removed it for now, we can add it later.  We should think about how to 
express what we support.  It's not enough to say we support the 
`didChangeConfiguration` notification, we should also express what element of 
configuration we support (the location of the compile commands directory, in 
this case).



Comment at: clangd/ClangdLSPServer.cpp:302
 
+// FIXME: This function needs to be properly tested.
+void ClangdLSPServer::onChangeConfiguration(

simark wrote:
> ilya-biryukov wrote:
> > simark wrote:
> > > ilya-biryukov wrote:
> > > > simark wrote:
> > > > > ilya-biryukov wrote:
> > > > > > Are you planning to to address this FIXME before checking the code 
> > > > > > in?
> > > > > Following what you said here:
> > > > > 
> > > > > https://reviews.llvm.org/D39571?id=124024#inline-359345
> > > > > 
> > > > > I have not really looked into what was wrong with the test, and what 
> > > > > is missing in the infrastructure to make it work.  But I assumed that 
> > > > > the situation did not change since then.  Can you enlighten me on 
> > > > > what the problem was, and what is missing?
> > > > We usually write unittests for that kind of thing, since they allow to 
> > > > plug an in-memory filesystem, but we only test `ClangdServer` (examples 
> > > > are in `unittests/clangd/ClangdTests.cpp`). `ClangdLSPServer` does not 
> > > > allow to plug in a virtual filesystem (vfs). Even if we add vfs, it's 
> > > > still hard to unit-test because we'll have to match the json 
> > > > input/output directly.
> > > > 
> > > > This leaves us with an option of a lit test that runs `clangd` 
> > > > directly, similar to tests in `test/clangd`.
> > > > The lit test would need to create a temporary directory, create proper 
> > > > `compile_commands.json` there, then send the LSP commands with the path 
> > > > to the test to clangd.
> > > > One major complication is that in LSP we have to specify the size of 
> > > > each message, but in our case the size would change depending on 
> > > > created temp path. It means we'll have to patch the test input to setup 
> > > > proper paths and message sizes.
> > > > If we choose to go down this path, 
> > > > `clang-tools-extra/test/clang-tidy/vfsoverlay.cpp` does a similar setup 
> > > > (create temp-dir, patch up some configuration files to point into the 
> > > > temp directory, etc) and could be used as a starting point.
> > > > 
> > > > It's not impossible to write that test, it's just a bit involved. 
> > > > Having a test would be nice, though, to ensure we don't break this 
> > > > method while doing other things. Especially given that this 
> > > > functionality is not used anywhere in clangd.
> > > > We usually write unittests for that kind of thing, since they allow to 
> > > > plug an in-memory filesystem, but we only test ClangdServer (examples 
> > > > are in unittests/clangd/ClangdTests.cpp). ClangdLSPServer does not 
> > > > allow to plug in a virtual filesystem (vfs). Even if we add vfs, it's 
> > > > still hard to unit-test because we'll have to match the json 
> > > > input/output directly.
> > > 
> > > What do you mean by "we'll have to match the json input/output directly"? 
> > >  That we'll have to match the complete JSON output textually?  Couldn't 
> > > the test parse the JSON into some data structures, then we could assert 
> > > specific things, like that this particular field is present and contains 
> > > a certain substring, for example?
> > > 
> > > > This leaves us with an option of a lit test that runs clangd directly, 
> > > > similar to tests in test/clangd.
> > > > The lit test would need to create a temporary directory, create proper 
> > > > 

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Simon Marchi via Phabricator via cfe-commits]

simark added inline comments.



Comment at: clangd/ClangdLSPServer.cpp:302
 
+// FIXME: This function needs to be properly tested.
+void ClangdLSPServer::onChangeConfiguration(

ilya-biryukov wrote:
> simark wrote:
> > ilya-biryukov wrote:
> > > simark wrote:
> > > > ilya-biryukov wrote:
> > > > > Are you planning to to address this FIXME before checking the code in?
> > > > Following what you said here:
> > > > 
> > > > https://reviews.llvm.org/D39571?id=124024#inline-359345
> > > > 
> > > > I have not really looked into what was wrong with the test, and what is 
> > > > missing in the infrastructure to make it work.  But I assumed that the 
> > > > situation did not change since then.  Can you enlighten me on what the 
> > > > problem was, and what is missing?
> > > We usually write unittests for that kind of thing, since they allow to 
> > > plug an in-memory filesystem, but we only test `ClangdServer` (examples 
> > > are in `unittests/clangd/ClangdTests.cpp`). `ClangdLSPServer` does not 
> > > allow to plug in a virtual filesystem (vfs). Even if we add vfs, it's 
> > > still hard to unit-test because we'll have to match the json input/output 
> > > directly.
> > > 
> > > This leaves us with an option of a lit test that runs `clangd` directly, 
> > > similar to tests in `test/clangd`.
> > > The lit test would need to create a temporary directory, create proper 
> > > `compile_commands.json` there, then send the LSP commands with the path 
> > > to the test to clangd.
> > > One major complication is that in LSP we have to specify the size of each 
> > > message, but in our case the size would change depending on created temp 
> > > path. It means we'll have to patch the test input to setup proper paths 
> > > and message sizes.
> > > If we choose to go down this path, 
> > > `clang-tools-extra/test/clang-tidy/vfsoverlay.cpp` does a similar setup 
> > > (create temp-dir, patch up some configuration files to point into the 
> > > temp directory, etc) and could be used as a starting point.
> > > 
> > > It's not impossible to write that test, it's just a bit involved. Having 
> > > a test would be nice, though, to ensure we don't break this method while 
> > > doing other things. Especially given that this functionality is not used 
> > > anywhere in clangd.
> > > We usually write unittests for that kind of thing, since they allow to 
> > > plug an in-memory filesystem, but we only test ClangdServer (examples are 
> > > in unittests/clangd/ClangdTests.cpp). ClangdLSPServer does not allow to 
> > > plug in a virtual filesystem (vfs). Even if we add vfs, it's still hard 
> > > to unit-test because we'll have to match the json input/output directly.
> > 
> > What do you mean by "we'll have to match the json input/output directly"?  
> > That we'll have to match the complete JSON output textually?  Couldn't the 
> > test parse the JSON into some data structures, then we could assert 
> > specific things, like that this particular field is present and contains a 
> > certain substring, for example?
> > 
> > > This leaves us with an option of a lit test that runs clangd directly, 
> > > similar to tests in test/clangd.
> > > The lit test would need to create a temporary directory, create proper 
> > > compile_commands.json there, then send the LSP commands with the path to 
> > > the test to clangd.
> > > One major complication is that in LSP we have to specify the size of each 
> > > message, but in our case the size would change depending on created temp 
> > > path. It means we'll have to patch the test input to setup proper paths 
> > > and message sizes.
> > > If we choose to go down this path, 
> > > clang-tools-extra/test/clang-tidy/vfsoverlay.cpp does a similar setup 
> > > (create temp-dir, patch up some configuration files to point into the 
> > > temp directory, etc) and could be used as a starting point.
> > 
> > Ok, I see the complication with the Content-Length.  I am not familiar with 
> > lit yet, so I don't know what it is capable of.  But being able to craft 
> > and send arbitrary LSP messages would certainly be helpful in the future 
> > for all kinds of black box test, so having a framework that allows to do 
> > this would be helpful, I think.  I'm not familiar enough with the ecosystem 
> > to do this right now, but I'll keep it in mind.
> > 
> > One question about this particular test.  Would there be some race 
> > condition here?  If we do:
> > 
> > 1. Start clangd with compile_commands.json #1
> > 2. Ask for the definition of a function, expecting a result
> > 3. Change the configuration to compile_commands.json #2
> > 4. Ask for the definition of the same function, expecting a different result
> > 
> > Since clangd is multi-threaded and does work in the background, are we sure 
> > that we'll get the result we want in #4?
> > 
> > > It's not impossible to write that test, it's just a bit involved. Having 
> > > a test would be nice, though, to ensure 

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/ClangdLSPServer.cpp:78
 {"documentHighlightProvider", true},
+{"configurationChangeProvider", true},
 {"renameProvider", true},

simark wrote:
> Nebiroth wrote:
> > simark wrote:
> > > I find `configurationChangeProvider` a bit weird.  It makes it sound like 
> > > clangd can provide configuration changes.  In reality, it can accept 
> > > configuration changes.  So I think this should be named something else.
> > Agreed, perhaps configurationChangeManager would be more appropriate then?
> I'm thinking of removing it for the time being.  Since it's not defined in 
> the protocol what types of configuration changes exist (it's specific to each 
> language server), it not very useful to simply advertise that we support 
> configuration changes.  We would need to advertise that we support 
> compilation database changes in particular.  I think this can be done later.
`"configurationChangeProvider"` is not in the LSP, right?
There's `experimental` field in the specification, let's put it under that 
field if you want to advertise that clangd supports this spec to your clients.



Comment at: clangd/ClangdLSPServer.cpp:302
 
+// FIXME: This function needs to be properly tested.
+void ClangdLSPServer::onChangeConfiguration(

simark wrote:
> ilya-biryukov wrote:
> > simark wrote:
> > > ilya-biryukov wrote:
> > > > Are you planning to to address this FIXME before checking the code in?
> > > Following what you said here:
> > > 
> > > https://reviews.llvm.org/D39571?id=124024#inline-359345
> > > 
> > > I have not really looked into what was wrong with the test, and what is 
> > > missing in the infrastructure to make it work.  But I assumed that the 
> > > situation did not change since then.  Can you enlighten me on what the 
> > > problem was, and what is missing?
> > We usually write unittests for that kind of thing, since they allow to plug 
> > an in-memory filesystem, but we only test `ClangdServer` (examples are in 
> > `unittests/clangd/ClangdTests.cpp`). `ClangdLSPServer` does not allow to 
> > plug in a virtual filesystem (vfs). Even if we add vfs, it's still hard to 
> > unit-test because we'll have to match the json input/output directly.
> > 
> > This leaves us with an option of a lit test that runs `clangd` directly, 
> > similar to tests in `test/clangd`.
> > The lit test would need to create a temporary directory, create proper 
> > `compile_commands.json` there, then send the LSP commands with the path to 
> > the test to clangd.
> > One major complication is that in LSP we have to specify the size of each 
> > message, but in our case the size would change depending on created temp 
> > path. It means we'll have to patch the test input to setup proper paths and 
> > message sizes.
> > If we choose to go down this path, 
> > `clang-tools-extra/test/clang-tidy/vfsoverlay.cpp` does a similar setup 
> > (create temp-dir, patch up some configuration files to point into the temp 
> > directory, etc) and could be used as a starting point.
> > 
> > It's not impossible to write that test, it's just a bit involved. Having a 
> > test would be nice, though, to ensure we don't break this method while 
> > doing other things. Especially given that this functionality is not used 
> > anywhere in clangd.
> > We usually write unittests for that kind of thing, since they allow to plug 
> > an in-memory filesystem, but we only test ClangdServer (examples are in 
> > unittests/clangd/ClangdTests.cpp). ClangdLSPServer does not allow to plug 
> > in a virtual filesystem (vfs). Even if we add vfs, it's still hard to 
> > unit-test because we'll have to match the json input/output directly.
> 
> What do you mean by "we'll have to match the json input/output directly"?  
> That we'll have to match the complete JSON output textually?  Couldn't the 
> test parse the JSON into some data structures, then we could assert specific 
> things, like that this particular field is present and contains a certain 
> substring, for example?
> 
> > This leaves us with an option of a lit test that runs clangd directly, 
> > similar to tests in test/clangd.
> > The lit test would need to create a temporary directory, create proper 
> > compile_commands.json there, then send the LSP commands with the path to 
> > the test to clangd.
> > One major complication is that in LSP we have to specify the size of each 
> > message, but in our case the size would change depending on created temp 
> > path. It means we'll have to patch the test input to setup proper paths and 
> > message sizes.
> > If we choose to go down this path, 
> > clang-tools-extra/test/clang-tidy/vfsoverlay.cpp does a similar setup 
> > (create temp-dir, patch up some configuration files to point into the temp 
> > directory, etc) and could be used as a starting point.
> 
> Ok, I see the complication with the 

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Simon Marchi via Phabricator via cfe-commits]

simark added inline comments.



Comment at: clangd/ClangdLSPServer.cpp:302
 
+// FIXME: This function needs to be properly tested.
+void ClangdLSPServer::onChangeConfiguration(

ilya-biryukov wrote:
> simark wrote:
> > ilya-biryukov wrote:
> > > Are you planning to to address this FIXME before checking the code in?
> > Following what you said here:
> > 
> > https://reviews.llvm.org/D39571?id=124024#inline-359345
> > 
> > I have not really looked into what was wrong with the test, and what is 
> > missing in the infrastructure to make it work.  But I assumed that the 
> > situation did not change since then.  Can you enlighten me on what the 
> > problem was, and what is missing?
> We usually write unittests for that kind of thing, since they allow to plug 
> an in-memory filesystem, but we only test `ClangdServer` (examples are in 
> `unittests/clangd/ClangdTests.cpp`). `ClangdLSPServer` does not allow to plug 
> in a virtual filesystem (vfs). Even if we add vfs, it's still hard to 
> unit-test because we'll have to match the json input/output directly.
> 
> This leaves us with an option of a lit test that runs `clangd` directly, 
> similar to tests in `test/clangd`.
> The lit test would need to create a temporary directory, create proper 
> `compile_commands.json` there, then send the LSP commands with the path to 
> the test to clangd.
> One major complication is that in LSP we have to specify the size of each 
> message, but in our case the size would change depending on created temp 
> path. It means we'll have to patch the test input to setup proper paths and 
> message sizes.
> If we choose to go down this path, 
> `clang-tools-extra/test/clang-tidy/vfsoverlay.cpp` does a similar setup 
> (create temp-dir, patch up some configuration files to point into the temp 
> directory, etc) and could be used as a starting point.
> 
> It's not impossible to write that test, it's just a bit involved. Having a 
> test would be nice, though, to ensure we don't break this method while doing 
> other things. Especially given that this functionality is not used anywhere 
> in clangd.
> We usually write unittests for that kind of thing, since they allow to plug 
> an in-memory filesystem, but we only test ClangdServer (examples are in 
> unittests/clangd/ClangdTests.cpp). ClangdLSPServer does not allow to plug in 
> a virtual filesystem (vfs). Even if we add vfs, it's still hard to unit-test 
> because we'll have to match the json input/output directly.

What do you mean by "we'll have to match the json input/output directly"?  That 
we'll have to match the complete JSON output textually?  Couldn't the test 
parse the JSON into some data structures, then we could assert specific things, 
like that this particular field is present and contains a certain substring, 
for example?

> This leaves us with an option of a lit test that runs clangd directly, 
> similar to tests in test/clangd.
> The lit test would need to create a temporary directory, create proper 
> compile_commands.json there, then send the LSP commands with the path to the 
> test to clangd.
> One major complication is that in LSP we have to specify the size of each 
> message, but in our case the size would change depending on created temp 
> path. It means we'll have to patch the test input to setup proper paths and 
> message sizes.
> If we choose to go down this path, 
> clang-tools-extra/test/clang-tidy/vfsoverlay.cpp does a similar setup (create 
> temp-dir, patch up some configuration files to point into the temp directory, 
> etc) and could be used as a starting point.

Ok, I see the complication with the Content-Length.  I am not familiar with lit 
yet, so I don't know what it is capable of.  But being able to craft and send 
arbitrary LSP messages would certainly be helpful in the future for all kinds 
of black box test, so having a framework that allows to do this would be 
helpful, I think.  I'm not familiar enough with the ecosystem to do this right 
now, but I'll keep it in mind.

One question about this particular test.  Would there be some race condition 
here?  If we do:

1. Start clangd with compile_commands.json #1
2. Ask for the definition of a function, expecting a result
3. Change the configuration to compile_commands.json #2
4. Ask for the definition of the same function, expecting a different result

Since clangd is multi-threaded and does work in the background, are we sure 
that we'll get the result we want in #4?

> It's not impossible to write that test, it's just a bit involved. Having a 
> test would be nice, though, to ensure we don't break this method while doing 
> other things. Especially given that this functionality is not used anywhere 
> in clangd.

I agree.  For the time being, is it fine to leave the FIXME there?  We can work 
on improving the test frameworks to get rid of it.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D39571

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/ClangdLSPServer.cpp:302
 
+// FIXME: This function needs to be properly tested.
+void ClangdLSPServer::onChangeConfiguration(

simark wrote:
> ilya-biryukov wrote:
> > Are you planning to to address this FIXME before checking the code in?
> Following what you said here:
> 
> https://reviews.llvm.org/D39571?id=124024#inline-359345
> 
> I have not really looked into what was wrong with the test, and what is 
> missing in the infrastructure to make it work.  But I assumed that the 
> situation did not change since then.  Can you enlighten me on what the 
> problem was, and what is missing?
We usually write unittests for that kind of thing, since they allow to plug an 
in-memory filesystem, but we only test `ClangdServer` (examples are in 
`unittests/clangd/ClangdTests.cpp`). `ClangdLSPServer` does not allow to plug 
in a virtual filesystem (vfs). Even if we add vfs, it's still hard to unit-test 
because we'll have to match the json input/output directly.

This leaves us with an option of a lit test that runs `clangd` directly, 
similar to tests in `test/clangd`.
The lit test would need to create a temporary directory, create proper 
`compile_commands.json` there, then send the LSP commands with the path to the 
test to clangd.
One major complication is that in LSP we have to specify the size of each 
message, but in our case the size would change depending on created temp path. 
It means we'll have to patch the test input to setup proper paths and message 
sizes.
If we choose to go down this path, 
`clang-tools-extra/test/clang-tidy/vfsoverlay.cpp` does a similar setup (create 
temp-dir, patch up some configuration files to point into the temp directory, 
etc) and could be used as a starting point.

It's not impossible to write that test, it's just a bit involved. Having a test 
would be nice, though, to ensure we don't break this method while doing other 
things. Especially given that this functionality is not used anywhere in clangd.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D39571



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

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 131305.
simark added a comment.

Move implementation of setCompileCommandsDir to .cpp


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D39571

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/DraftStore.cpp
  clangd/DraftStore.h
  clangd/GlobalCompilationDatabase.cpp
  clangd/GlobalCompilationDatabase.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h

Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -57,6 +57,8 @@
   virtual void onRename(Ctx C, RenameParams ) = 0;
   virtual void onDocumentHighlight(Ctx C,
TextDocumentPositionParams ) = 0;
+  virtual void onChangeConfiguration(Ctx C,
+ DidChangeConfigurationParams ) = 0;
 };
 
 void registerCallbackHandlers(JSONRPCDispatcher , JSONOutput ,
Index: clangd/ProtocolHandlers.cpp
===
--- clangd/ProtocolHandlers.cpp
+++ clangd/ProtocolHandlers.cpp
@@ -73,4 +73,6 @@
   Register("workspace/executeCommand", ::onCommand);
   Register("textDocument/documentHighlight",
::onDocumentHighlight);
+  Register("workspace/didChangeConfiguration",
+   ::onChangeConfiguration);
 }
Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -264,6 +264,20 @@
 };
 bool fromJSON(const json::Expr &, DidChangeWatchedFilesParams &);
 
+/// Clangd extension to manage a workspace/didChangeConfiguration notification
+/// since the data received is described as 'any' type in LSP.
+struct ClangdConfigurationParamsChange {
+  llvm::Optional compilationDatabasePath;
+};
+bool fromJSON(const json::Expr &, ClangdConfigurationParamsChange &);
+
+struct DidChangeConfigurationParams {
+  // We use this predefined struct because it is easier to use
+  // than the protocol specified type of 'any'.
+  ClangdConfigurationParamsChange settings;
+};
+bool fromJSON(const json::Expr &, DidChangeConfigurationParams &);
+
 struct FormattingOptions {
   /// Size of a tab in spaces.
   int tabSize;
Index: clangd/Protocol.cpp
===
--- clangd/Protocol.cpp
+++ clangd/Protocol.cpp
@@ -381,5 +381,15 @@
   };
 }
 
+bool fromJSON(const json::Expr , DidChangeConfigurationParams ) {
+  json::ObjectMapper O(Params);
+  return O && O.map("settings", CCP.settings);
+}
+
+bool fromJSON(const json::Expr , ClangdConfigurationParamsChange ) {
+  json::ObjectMapper O(Params);
+  return O && O.map("compilationDatabasePath", CCPC.compilationDatabasePath);
+}
+
 } // namespace clangd
 } // namespace clang
Index: clangd/GlobalCompilationDatabase.h
===
--- clangd/GlobalCompilationDatabase.h
+++ clangd/GlobalCompilationDatabase.h
@@ -61,6 +61,8 @@
   /// Uses the default fallback command, adding any extra flags.
   tooling::CompileCommand getFallbackCommand(PathRef File) const override;
 
+  void setCompileCommandsDir(Path P);
+
   /// Sets the extra flags that should be added to a file.
   void setExtraFlagsForFile(PathRef File, std::vector ExtraFlags);
 
Index: clangd/GlobalCompilationDatabase.cpp
===
--- clangd/GlobalCompilationDatabase.cpp
+++ clangd/GlobalCompilationDatabase.cpp
@@ -52,6 +52,12 @@
   return C;
 }
 
+void DirectoryBasedGlobalCompilationDatabase::setCompileCommandsDir(Path P) {
+  std::lock_guard Lock(Mutex);
+  CompileCommandsDir = P;
+  CompilationDatabases.clear();
+}
+
 void DirectoryBasedGlobalCompilationDatabase::setExtraFlagsForFile(
 PathRef File, std::vector ExtraFlags) {
   std::lock_guard Lock(Mutex);
Index: clangd/DraftStore.h
===
--- clangd/DraftStore.h
+++ clangd/DraftStore.h
@@ -40,6 +40,10 @@
   /// \return version and contents of the stored document.
   /// For untracked files, a (0, None) pair is returned.
   VersionedDraft getDraft(PathRef File) const;
+
+  /// \return List of names of drafts in this store.
+  std::vector getActiveFiles() const;
+
   /// \return version of the tracked document.
   /// For untracked files, 0 is returned.
   DocVersion getVersion(PathRef File) const;
Index: clangd/DraftStore.cpp
===
--- clangd/DraftStore.cpp
+++ clangd/DraftStore.cpp
@@ -21,6 +21,16 @@
   return It->second;
 }
 
+std::vector DraftStore::getActiveFiles() const {
+  std::lock_guard Lock(Mutex);
+  std::vector ResultVector;
+
+  for (auto Draft : Drafts.keys())
+ResultVector.push_back(Draft);
+
+  return ResultVector;
+}
+
 DocVersion 

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Simon Marchi via Phabricator via cfe-commits]

simark added inline comments.



Comment at: clangd/ClangdLSPServer.cpp:302
 
+// FIXME: This function needs to be properly tested.
+void ClangdLSPServer::onChangeConfiguration(

ilya-biryukov wrote:
> Are you planning to to address this FIXME before checking the code in?
Following what you said here:

https://reviews.llvm.org/D39571?id=124024#inline-359345

I have not really looked into what was wrong with the test, and what is missing 
in the infrastructure to make it work.  But I assumed that the situation did 
not change since then.  Can you enlighten me on what the problem was, and what 
is missing?



Comment at: clangd/GlobalCompilationDatabase.h:64
 
+  void setCompileCommandsDir(Path P) {
+std::lock_guard Lock(Mutex);

ilya-biryukov wrote:
> Maybe move definition to `.cpp` file?
> 
> 
Will do.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D39571



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

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Hi @simark , thanks for picking up this change.




Comment at: clangd/ClangdLSPServer.cpp:302
 
+// FIXME: This function needs to be properly tested.
+void ClangdLSPServer::onChangeConfiguration(

Are you planning to to address this FIXME before checking the code in?



Comment at: clangd/GlobalCompilationDatabase.h:64
 
+  void setCompileCommandsDir(Path P) {
+std::lock_guard Lock(Mutex);

Maybe move definition to `.cpp` file?




Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D39571



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

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 131131.
simark added a comment.

Fix formatting, remove capability

- Fix some formatting nits
- Remove the entry in the capabilities object


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D39571

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/DraftStore.cpp
  clangd/DraftStore.h
  clangd/GlobalCompilationDatabase.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h

Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -57,6 +57,8 @@
   virtual void onRename(Ctx C, RenameParams ) = 0;
   virtual void onDocumentHighlight(Ctx C,
TextDocumentPositionParams ) = 0;
+  virtual void onChangeConfiguration(Ctx C,
+ DidChangeConfigurationParams ) = 0;
 };
 
 void registerCallbackHandlers(JSONRPCDispatcher , JSONOutput ,
Index: clangd/ProtocolHandlers.cpp
===
--- clangd/ProtocolHandlers.cpp
+++ clangd/ProtocolHandlers.cpp
@@ -73,4 +73,6 @@
   Register("workspace/executeCommand", ::onCommand);
   Register("textDocument/documentHighlight",
::onDocumentHighlight);
+  Register("workspace/didChangeConfiguration",
+   ::onChangeConfiguration);
 }
Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -264,6 +264,20 @@
 };
 bool fromJSON(const json::Expr &, DidChangeWatchedFilesParams &);
 
+/// Clangd extension to manage a workspace/didChangeConfiguration notification
+/// since the data received is described as 'any' type in LSP.
+struct ClangdConfigurationParamsChange {
+  llvm::Optional compilationDatabasePath;
+};
+bool fromJSON(const json::Expr &, ClangdConfigurationParamsChange &);
+
+struct DidChangeConfigurationParams {
+  // We use this predefined struct because it is easier to use
+  // than the protocol specified type of 'any'.
+  ClangdConfigurationParamsChange settings;
+};
+bool fromJSON(const json::Expr &, DidChangeConfigurationParams &);
+
 struct FormattingOptions {
   /// Size of a tab in spaces.
   int tabSize;
Index: clangd/Protocol.cpp
===
--- clangd/Protocol.cpp
+++ clangd/Protocol.cpp
@@ -381,5 +381,15 @@
   };
 }
 
+bool fromJSON(const json::Expr , DidChangeConfigurationParams ) {
+  json::ObjectMapper O(Params);
+  return O && O.map("settings", CCP.settings);
+}
+
+bool fromJSON(const json::Expr , ClangdConfigurationParamsChange ) {
+  json::ObjectMapper O(Params);
+  return O && O.map("compilationDatabasePath", CCPC.compilationDatabasePath);
+}
+
 } // namespace clangd
 } // namespace clang
Index: clangd/GlobalCompilationDatabase.h
===
--- clangd/GlobalCompilationDatabase.h
+++ clangd/GlobalCompilationDatabase.h
@@ -61,6 +61,12 @@
   /// Uses the default fallback command, adding any extra flags.
   tooling::CompileCommand getFallbackCommand(PathRef File) const override;
 
+  void setCompileCommandsDir(Path P) {
+std::lock_guard Lock(Mutex);
+CompileCommandsDir = P;
+CompilationDatabases.clear();
+  }
+
   /// Sets the extra flags that should be added to a file.
   void setExtraFlagsForFile(PathRef File, std::vector ExtraFlags);
 
Index: clangd/DraftStore.h
===
--- clangd/DraftStore.h
+++ clangd/DraftStore.h
@@ -40,6 +40,10 @@
   /// \return version and contents of the stored document.
   /// For untracked files, a (0, None) pair is returned.
   VersionedDraft getDraft(PathRef File) const;
+
+  /// \return List of names of drafts in this store.
+  std::vector getActiveFiles() const;
+
   /// \return version of the tracked document.
   /// For untracked files, 0 is returned.
   DocVersion getVersion(PathRef File) const;
Index: clangd/DraftStore.cpp
===
--- clangd/DraftStore.cpp
+++ clangd/DraftStore.cpp
@@ -21,6 +21,16 @@
   return It->second;
 }
 
+std::vector DraftStore::getActiveFiles() const {
+  std::lock_guard Lock(Mutex);
+  std::vector ResultVector;
+
+  for (auto Draft : Drafts.keys())
+ResultVector.push_back(Draft);
+
+  return ResultVector;
+}
+
 DocVersion DraftStore::getVersion(PathRef File) const {
   std::lock_guard Lock(Mutex);
 
Index: clangd/ClangdServer.h
===
--- clangd/ClangdServer.h
+++ clangd/ClangdServer.h
@@ -233,6 +233,11 @@
   /// and AST and rebuild them from scratch.
   std::future forceReparse(Context Ctx, PathRef File);
 
+  /// Calls forceReparse() on all currently opened files.
+  /// As a result, this method may be 

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Simon Marchi via Phabricator via cfe-commits]

simark added inline comments.



Comment at: clangd/ClangdLSPServer.cpp:78
 {"documentHighlightProvider", true},
+{"configurationChangeProvider", true},
 {"renameProvider", true},

Nebiroth wrote:
> simark wrote:
> > I find `configurationChangeProvider` a bit weird.  It makes it sound like 
> > clangd can provide configuration changes.  In reality, it can accept 
> > configuration changes.  So I think this should be named something else.
> Agreed, perhaps configurationChangeManager would be more appropriate then?
I'm thinking of removing it for the time being.  Since it's not defined in the 
protocol what types of configuration changes exist (it's specific to each 
language server), it not very useful to simply advertise that we support 
configuration changes.  We would need to advertise that we support compilation 
database changes in particular.  I think this can be done later.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D39571



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

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[William Enright via Phabricator via cfe-commits]

Nebiroth added inline comments.



Comment at: clangd/ClangdLSPServer.cpp:78
 {"documentHighlightProvider", true},
+{"configurationChangeProvider", true},
 {"renameProvider", true},

simark wrote:
> I find `configurationChangeProvider` a bit weird.  It makes it sound like 
> clangd can provide configuration changes.  In reality, it can accept 
> configuration changes.  So I think this should be named something else.
Agreed, perhaps configurationChangeManager would be more appropriate then?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D39571



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

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Simon Marchi via Phabricator via cfe-commits]

simark added inline comments.



Comment at: clangd/ClangdLSPServer.cpp:78
 {"documentHighlightProvider", true},
+{"configurationChangeProvider", true},
 {"renameProvider", true},

I find `configurationChangeProvider` a bit weird.  It makes it sound like 
clangd can provide configuration changes.  In reality, it can accept 
configuration changes.  So I think this should be named something else.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D39571



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

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 131051.
simark added a comment.
Herald added subscribers: ioeric, jkorous-apple.

I just got familiar with the patch, I cleaned up the bits that I thought were
unnecessary for this change.  For example, we don't need a toJSON function for
DidChangeConfigurationParams, or explicit constructors to
DidChangeConfigurationParams, etc.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D39571

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/DraftStore.cpp
  clangd/DraftStore.h
  clangd/GlobalCompilationDatabase.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  test/clangd/initialize-params-invalid.test
  test/clangd/initialize-params.test

Index: test/clangd/initialize-params.test
===
--- test/clangd/initialize-params.test
+++ test/clangd/initialize-params.test
@@ -18,9 +18,10 @@
 # CHECK-NEXT:  ":"
 # CHECK-NEXT:]
 # CHECK-NEXT:  },
+# CHECK-NEXT:  "configurationChangeProvider": true,
 # CHECK-NEXT:  "definitionProvider": true,
 # CHECK-NEXT:  "documentFormattingProvider": true,
-# CHECK-NEXT:  "documentHighlightProvider": true,
+# CHECK-NEXT:	   "documentHighlightProvider": true,
 # CHECK-NEXT:  "documentOnTypeFormattingProvider": {
 # CHECK-NEXT:"firstTriggerCharacter": "}",
 # CHECK-NEXT:"moreTriggerCharacter": []
@@ -49,4 +50,4 @@
 # CHECK-NEXT:  "result": null
 Content-Length: 33
 
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0":"method":"exit"}
\ No newline at end of file
Index: test/clangd/initialize-params-invalid.test
===
--- test/clangd/initialize-params-invalid.test
+++ test/clangd/initialize-params-invalid.test
@@ -18,9 +18,10 @@
 # CHECK-NEXT:  ":"
 # CHECK-NEXT:]
 # CHECK-NEXT:  },
+# CHECK-NEXT:  "configurationChangeProvider": true,
 # CHECK-NEXT:  "definitionProvider": true,
 # CHECK-NEXT:  "documentFormattingProvider": true,
-# CHECK-NEXT:  "documentHighlightProvider": true,
+# CHECK-NEXT:	   "documentHighlightProvider": true,
 # CHECK-NEXT:  "documentOnTypeFormattingProvider": {
 # CHECK-NEXT:"firstTriggerCharacter": "}",
 # CHECK-NEXT:"moreTriggerCharacter": []
Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -57,6 +57,8 @@
   virtual void onRename(Ctx C, RenameParams ) = 0;
   virtual void onDocumentHighlight(Ctx C,
TextDocumentPositionParams ) = 0;
+  virtual void onChangeConfiguration(Ctx C,
+ DidChangeConfigurationParams ) = 0;
 };
 
 void registerCallbackHandlers(JSONRPCDispatcher , JSONOutput ,
Index: clangd/ProtocolHandlers.cpp
===
--- clangd/ProtocolHandlers.cpp
+++ clangd/ProtocolHandlers.cpp
@@ -73,4 +73,6 @@
   Register("workspace/executeCommand", ::onCommand);
   Register("textDocument/documentHighlight",
::onDocumentHighlight);
+  Register("workspace/didChangeConfiguration",
+   ::onChangeConfiguration);
 }
Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -264,6 +264,20 @@
 };
 bool fromJSON(const json::Expr &, DidChangeWatchedFilesParams &);
 
+/// Clangd extension to manage a workspace/didChangeConfiguration notification
+/// since the data received is described as 'any' type in LSP.
+struct ClangdConfigurationParamsChange {
+  llvm::Optional compilationDatabasePath;
+};
+bool fromJSON(const json::Expr &, ClangdConfigurationParamsChange &);
+
+struct DidChangeConfigurationParams {
+  // We use this predefined struct because it is easier to use
+  // than the protocol specified type of 'any'.
+  ClangdConfigurationParamsChange settings;
+};
+bool fromJSON(const json::Expr &, DidChangeConfigurationParams &);
+
 struct FormattingOptions {
   /// Size of a tab in spaces.
   int tabSize;
Index: clangd/Protocol.cpp
===
--- clangd/Protocol.cpp
+++ clangd/Protocol.cpp
@@ -381,5 +381,16 @@
   };
 }
 
+bool fromJSON(const json::Expr , DidChangeConfigurationParams ) {
+  json::ObjectMapper O(Params);
+  return O && O.map("settings", CCP.settings);
+}
+
+
+bool fromJSON(const json::Expr , ClangdConfigurationParamsChange ) {
+  json::ObjectMapper O(Params);
+  return O && O.map("compilationDatabasePath", CCPC.compilationDatabasePath);
+}
+
 } // namespace clangd
 } // namespace clang
Index: clangd/GlobalCompilationDatabase.h
===
--- clangd/GlobalCompilationDatabase.h
+++ 

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[William Enright via Phabricator via cfe-commits]

Nebiroth updated this revision to Diff 126980.
Nebiroth marked 5 inline comments as done.
Nebiroth added a comment.

Removed test file
Added mutex lock when changing CDB
Minor code cleanup


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D39571

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/DraftStore.cpp
  clangd/DraftStore.h
  clangd/GlobalCompilationDatabase.cpp
  clangd/GlobalCompilationDatabase.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  test/clangd/initialize-params-invalid.test
  test/clangd/initialize-params.test

Index: test/clangd/initialize-params.test
===
--- test/clangd/initialize-params.test
+++ test/clangd/initialize-params.test
@@ -18,9 +18,10 @@
 # CHECK-NEXT:  ":"
 # CHECK-NEXT:]
 # CHECK-NEXT:  },
+# CHECK-NEXT:  "configurationChangeProvider": true,
 # CHECK-NEXT:  "definitionProvider": true,
 # CHECK-NEXT:  "documentFormattingProvider": true,
-# CHECK-NEXT:  "documentHighlightProvider": true,
+# CHECK-NEXT:	   "documentHighlightProvider": true,
 # CHECK-NEXT:  "documentOnTypeFormattingProvider": {
 # CHECK-NEXT:"firstTriggerCharacter": "}",
 # CHECK-NEXT:"moreTriggerCharacter": []
@@ -49,4 +50,4 @@
 # CHECK-NEXT:  "result": null
 Content-Length: 33
 
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0":"method":"exit"}
\ No newline at end of file
Index: test/clangd/initialize-params-invalid.test
===
--- test/clangd/initialize-params-invalid.test
+++ test/clangd/initialize-params-invalid.test
@@ -18,9 +18,10 @@
 # CHECK-NEXT:  ":"
 # CHECK-NEXT:]
 # CHECK-NEXT:  },
+# CHECK-NEXT:  "configurationChangeProvider": true,
 # CHECK-NEXT:  "definitionProvider": true,
 # CHECK-NEXT:  "documentFormattingProvider": true,
-# CHECK-NEXT:  "documentHighlightProvider": true,
+# CHECK-NEXT:	   "documentHighlightProvider": true,
 # CHECK-NEXT:  "documentOnTypeFormattingProvider": {
 # CHECK-NEXT:"firstTriggerCharacter": "}",
 # CHECK-NEXT:"moreTriggerCharacter": []
Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -57,6 +57,8 @@
   virtual void onRename(Ctx C, RenameParams ) = 0;
   virtual void onDocumentHighlight(Ctx C,
TextDocumentPositionParams ) = 0;
+  virtual void onChangeConfiguration(Ctx C,
+ DidChangeConfigurationParams ) = 0;
 };
 
 void registerCallbackHandlers(JSONRPCDispatcher , JSONOutput ,
Index: clangd/ProtocolHandlers.cpp
===
--- clangd/ProtocolHandlers.cpp
+++ clangd/ProtocolHandlers.cpp
@@ -73,4 +73,6 @@
   Register("workspace/executeCommand", ::onCommand);
   Register("textDocument/documentHighlight",
::onDocumentHighlight);
+  Register("workspace/didChangeConfiguration",
+   ::onChangeConfiguration);
 }
Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -257,6 +257,24 @@
 };
 bool fromJSON(const json::Expr &, DidChangeWatchedFilesParams &);
 
+/// Clangd extension to manage a workspace/didChangeConfiguration notification
+/// since the data received is described as 'any' type in LSP.
+struct ClangdConfigurationParamsChange {
+  llvm::Optional compilationDatabasePath;
+};
+bool fromJSON(const json::Expr &, ClangdConfigurationParamsChange &);
+
+struct DidChangeConfigurationParams {
+  DidChangeConfigurationParams() = default;
+  DidChangeConfigurationParams(ClangdConfigurationParamsChange settings): settings(settings) {}
+
+  // We use this predefined struct because it is easier to use
+  // than the protocol specified type of 'any'.
+  ClangdConfigurationParamsChange settings;
+};
+bool fromJSON(const json::Expr &, DidChangeConfigurationParams &);
+json::Expr toJSON(const DidChangeConfigurationParams &);
+
 struct FormattingOptions {
   /// Size of a tab in spaces.
   int tabSize;
Index: clangd/Protocol.cpp
===
--- clangd/Protocol.cpp
+++ clangd/Protocol.cpp
@@ -366,5 +366,21 @@
   };
 }
 
+bool fromJSON(const json::Expr , DidChangeConfigurationParams ) {
+  json::ObjectMapper O(Params);
+  return O && O.map("settings", CCP.settings);
+}
+
+json::Expr toJSON(const DidChangeConfigurationParams ) {
+  return json::obj{
+{"settings", CCP.settings},
+  };
+}
+
+bool fromJSON(const json::Expr , ClangdConfigurationParamsChange ) {
+  json::ObjectMapper O(Params);
+  return O && O.map("compilationDatabasePath", CCPC.compilationDatabasePath);
+}
+
 } // namespace clangd
 } // namespace 

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov requested changes to this revision.
ilya-biryukov added a comment.
This revision now requires changes to proceed.

It seems this patch is out of date, we need to merge it with the latests head.




Comment at: clangd/DraftStore.cpp:45
   std::lock_guard Lock(Mutex);
+  assert(!File.empty());
 

Why do we need this assert? I don't see how empty file is worse than any other 
invalid value.



Comment at: clangd/DraftStore.cpp:55
   std::lock_guard Lock(Mutex);
+  assert(!File.empty());
 

Why do we need this assert? I don't see how empty file is worse than any other 
invalid value.



Comment at: clangd/GlobalCompilationDatabase.h:65
+  void setCompileCommandsDir(Path P) {
+CompileCommandsDir = P;
+CompilationDatabases.clear();

We need to lock the Mutex here!



Comment at: clangd/Protocol.cpp:368
+json::Expr toJSON(const DidChangeConfigurationParams ) {
+
+  return json::obj{

NIT: maybe remove this empty line?



Comment at: test/clangd/did-change-configuration.test:33
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///compile_commands.json","languageId":"json","version":1,"text":"[\n{\n"directory":"/",\n"command":"/usr/bin/c++-DGTEST_HAS_RTTI=0-D_GNU_SOURCE-D__STDC_CONSTANT_MACROS-D__STDC_FORMAT_MACROS-D__STDC_LIMIT_MACROS-Ilib/Demangle-I../lib/Demangle-I/usr/include/libxml2-Iinclude-I../include-fPIC-fvisibility-inlines-hidden-Werror=date-time-std=c++11-Wall-W-Wno-unused-parameter-Wwrite-strings-Wcast-qual-Wno-missing-field-initializers-pedantic-Wno-long-long-Wno-maybe-uninitialized-Wdelete-non-virtual-dtor-Wno-comment-O0-g-fno-exceptions-fno-rtti-o/foo.c.o-c/foo.c",\n"file":"/foo.c"\n},"}}}
+

Nebiroth wrote:
> ilya-biryukov wrote:
> > clangd won't see this file. `didOpen` only sets contents for diagnostics, 
> > not any other features.
> > You would rather want to add more `# RUN:` directives at the top of the 
> > file to create `compile_commands.json`, etc.
> > 
> > Writing it under root ('/') is obviously not an option. Lit tests allow you 
> > to use temporary paths, this is probably an approach you could take. See [[ 
> > https://llvm.org/docs/TestingGuide.html#substitutions | lit docs ]] for 
> > more details.
> Are there examples available on how to use this? I have to use a # RUN: to 
> create a file and then use it's path in a workspace/didChangeConfiguration 
> notification?
After thinking about it, I really don't see an easy way to test it currently. 
We just don't have the infrastructure in place for that.
I think it's fine to add a FIXME to the body of 
`ClangdLSPServer::onChangeConfiguration` that we need to test it and remove 
this test.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D39571



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

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[William Enright via Phabricator via cfe-commits]

Nebiroth updated this revision to Diff 126449.
Nebiroth added a comment.

Removed some more empty lines


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D39571

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/DraftStore.cpp
  clangd/DraftStore.h
  clangd/GlobalCompilationDatabase.cpp
  clangd/GlobalCompilationDatabase.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  test/clangd/did-change-configuration.test
  test/clangd/initialize-params-invalid.test
  test/clangd/initialize-params.test

Index: test/clangd/initialize-params.test
===
--- test/clangd/initialize-params.test
+++ test/clangd/initialize-params.test
@@ -18,6 +18,7 @@
 # CHECK-NEXT:  ":"
 # CHECK-NEXT:]
 # CHECK-NEXT:  },
+# CHECK-NEXT:  "configurationChangeProvider": true,
 # CHECK-NEXT:  "definitionProvider": true,
 # CHECK-NEXT:  "documentFormattingProvider": true,
 # CHECK-NEXT:  "documentOnTypeFormattingProvider": {
Index: test/clangd/initialize-params-invalid.test
===
--- test/clangd/initialize-params-invalid.test
+++ test/clangd/initialize-params-invalid.test
@@ -18,6 +18,7 @@
 # CHECK-NEXT:  ":"
 # CHECK-NEXT:]
 # CHECK-NEXT:  },
+# CHECK-NEXT:  "configurationChangeProvider": true,
 # CHECK-NEXT:  "definitionProvider": true,
 # CHECK-NEXT:  "documentFormattingProvider": true,
 # CHECK-NEXT:  "documentOnTypeFormattingProvider": {
Index: test/clangd/did-change-configuration.test
===
--- /dev/null
+++ test/clangd/did-change-configuration.test
@@ -0,0 +1,46 @@
+# RUN: clangd -run-synchronously < %s | FileCheck %s
+# It is absolutely vital that this file has CRLF line endings.
+#
+Content-Length: 125
+
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+
+Content-Length: 150
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///foo.c","languageId":"c","version":1,"text":"voidmain(){}"}}}
+
+Content-Length: 86
+
+{"jsonrpc":"2.0","method":"workspace/didChangeConfiguration","params":{"settings":""}}
+#Failed to decode workspace/didChangeConfiguration request.
+#Incorrect mapping node
+
+Content-Length: 114
+
+{"jsonrpc":"2.0","method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":{
+#Failed to decode workspace/didChangeConfiguration request.
+#compilationDatabasePath is not a scalar node
+
+Content-Length: 140
+
+{"jsonrpc":"2.0","method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":"/","testBadValue":"foo1234"}}}
+#Ignored unknown field "testBadValue"
+#Failed to find compilation database for / in overriden directory /
+#Bad field, bad compilation database
+
+Content-Length: 722
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///compile_commands.json","languageId":"json","version":1,"text":"[\n{\n"directory":"/",\n"command":"/usr/bin/c++-DGTEST_HAS_RTTI=0-D_GNU_SOURCE-D__STDC_CONSTANT_MACROS-D__STDC_FORMAT_MACROS-D__STDC_LIMIT_MACROS-Ilib/Demangle-I../lib/Demangle-I/usr/include/libxml2-Iinclude-I../include-fPIC-fvisibility-inlines-hidden-Werror=date-time-std=c++11-Wall-W-Wno-unused-parameter-Wwrite-strings-Wcast-qual-Wno-missing-field-initializers-pedantic-Wno-long-long-Wno-maybe-uninitialized-Wdelete-non-virtual-dtor-Wno-comment-O0-g-fno-exceptions-fno-rtti-o/foo.c.o-c/foo.c",\n"file":"/foo.c"\n},"}}}
+
+Content-Length: 115
+
+{"jsonrpc":"2.0","method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":"/"}}}
+#CHECK:{"jsonrpc":"2.0","method":"textDocument/publishDiagnostics","params":{"diagnostics":[{"message":"type specifier missing, defaults to 'int'","range":{"end":{"character":1,"line":0},"start":{"character":1,"line":0}},"severity":2},{"message":"control reaches end of non-void function","range":{"end":{"character":12,"line":0},"start":{"character":12,"line":0}},"severity":2}],"uri":"file:///foo.c"}}
+
+Content-Length: 48
+
+{"jsonrpc":"2.0","id":1,"method":"shutdown"}
+
+Content-Length: 33
+
+{"jsonrpc":"2.0":"method":"exit"}
Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -54,6 +54,8 @@
   virtual void onFileEvent(Ctx C, DidChangeWatchedFilesParams ) = 0;
   virtual void onCommand(Ctx C, ExecuteCommandParams ) = 0;
   virtual void onRename(Ctx C, RenameParams ) = 0;
+  virtual void onChangeConfiguration(Ctx C,
+ DidChangeConfigurationParams ) = 0;
 };
 
 void registerCallbackHandlers(JSONRPCDispatcher , JSONOutput ,

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[William Enright via Phabricator via cfe-commits]

Nebiroth updated this revision to Diff 126447.
Nebiroth added a comment.
Herald added a subscriber: klimek.

Merged with latest llvm + clang
Minor code cleanup


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D39571

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/DraftStore.cpp
  clangd/DraftStore.h
  clangd/GlobalCompilationDatabase.cpp
  clangd/GlobalCompilationDatabase.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  test/clangd/did-change-configuration.test
  test/clangd/initialize-params-invalid.test
  test/clangd/initialize-params.test

Index: test/clangd/initialize-params.test
===
--- test/clangd/initialize-params.test
+++ test/clangd/initialize-params.test
@@ -18,6 +18,7 @@
 # CHECK-NEXT:  ":"
 # CHECK-NEXT:]
 # CHECK-NEXT:  },
+# CHECK-NEXT:  "configurationChangeProvider": true,
 # CHECK-NEXT:  "definitionProvider": true,
 # CHECK-NEXT:  "documentFormattingProvider": true,
 # CHECK-NEXT:  "documentOnTypeFormattingProvider": {
Index: test/clangd/initialize-params-invalid.test
===
--- test/clangd/initialize-params-invalid.test
+++ test/clangd/initialize-params-invalid.test
@@ -18,6 +18,7 @@
 # CHECK-NEXT:  ":"
 # CHECK-NEXT:]
 # CHECK-NEXT:  },
+# CHECK-NEXT:  "configurationChangeProvider": true,
 # CHECK-NEXT:  "definitionProvider": true,
 # CHECK-NEXT:  "documentFormattingProvider": true,
 # CHECK-NEXT:  "documentOnTypeFormattingProvider": {
Index: test/clangd/did-change-configuration.test
===
--- /dev/null
+++ test/clangd/did-change-configuration.test
@@ -0,0 +1,46 @@
+# RUN: clangd -run-synchronously < %s | FileCheck %s
+# It is absolutely vital that this file has CRLF line endings.
+#
+Content-Length: 125
+
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+
+Content-Length: 150
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///foo.c","languageId":"c","version":1,"text":"voidmain(){}"}}}
+
+Content-Length: 86
+
+{"jsonrpc":"2.0","method":"workspace/didChangeConfiguration","params":{"settings":""}}
+#Failed to decode workspace/didChangeConfiguration request.
+#Incorrect mapping node
+
+Content-Length: 114
+
+{"jsonrpc":"2.0","method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":{
+#Failed to decode workspace/didChangeConfiguration request.
+#compilationDatabasePath is not a scalar node
+
+Content-Length: 140
+
+{"jsonrpc":"2.0","method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":"/","testBadValue":"foo1234"}}}
+#Ignored unknown field "testBadValue"
+#Failed to find compilation database for / in overriden directory /
+#Bad field, bad compilation database
+
+Content-Length: 722
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///compile_commands.json","languageId":"json","version":1,"text":"[\n{\n"directory":"/",\n"command":"/usr/bin/c++-DGTEST_HAS_RTTI=0-D_GNU_SOURCE-D__STDC_CONSTANT_MACROS-D__STDC_FORMAT_MACROS-D__STDC_LIMIT_MACROS-Ilib/Demangle-I../lib/Demangle-I/usr/include/libxml2-Iinclude-I../include-fPIC-fvisibility-inlines-hidden-Werror=date-time-std=c++11-Wall-W-Wno-unused-parameter-Wwrite-strings-Wcast-qual-Wno-missing-field-initializers-pedantic-Wno-long-long-Wno-maybe-uninitialized-Wdelete-non-virtual-dtor-Wno-comment-O0-g-fno-exceptions-fno-rtti-o/foo.c.o-c/foo.c",\n"file":"/foo.c"\n},"}}}
+
+Content-Length: 115
+
+{"jsonrpc":"2.0","method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":"/"}}}
+#CHECK:{"jsonrpc":"2.0","method":"textDocument/publishDiagnostics","params":{"diagnostics":[{"message":"type specifier missing, defaults to 'int'","range":{"end":{"character":1,"line":0},"start":{"character":1,"line":0}},"severity":2},{"message":"control reaches end of non-void function","range":{"end":{"character":12,"line":0},"start":{"character":12,"line":0}},"severity":2}],"uri":"file:///foo.c"}}
+
+Content-Length: 48
+
+{"jsonrpc":"2.0","id":1,"method":"shutdown"}
+
+Content-Length: 33
+
+{"jsonrpc":"2.0":"method":"exit"}
Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -54,6 +54,8 @@
   virtual void onFileEvent(Ctx C, DidChangeWatchedFilesParams ) = 0;
   virtual void onCommand(Ctx C, ExecuteCommandParams ) = 0;
   virtual void onRename(Ctx C, RenameParams ) = 0;
+  virtual void onChangeConfiguration(Ctx C,
+ DidChangeConfigurationParams ) = 0;
 };
 
 void 

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[William Enright via Phabricator via cfe-commits]

Nebiroth marked 12 inline comments as done.
Nebiroth added inline comments.



Comment at: test/clangd/did-change-configuration.test:33
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///compile_commands.json","languageId":"json","version":1,"text":"[\n{\n"directory":"/",\n"command":"/usr/bin/c++-DGTEST_HAS_RTTI=0-D_GNU_SOURCE-D__STDC_CONSTANT_MACROS-D__STDC_FORMAT_MACROS-D__STDC_LIMIT_MACROS-Ilib/Demangle-I../lib/Demangle-I/usr/include/libxml2-Iinclude-I../include-fPIC-fvisibility-inlines-hidden-Werror=date-time-std=c++11-Wall-W-Wno-unused-parameter-Wwrite-strings-Wcast-qual-Wno-missing-field-initializers-pedantic-Wno-long-long-Wno-maybe-uninitialized-Wdelete-non-virtual-dtor-Wno-comment-O0-g-fno-exceptions-fno-rtti-o/foo.c.o-c/foo.c",\n"file":"/foo.c"\n},"}}}
+

ilya-biryukov wrote:
> clangd won't see this file. `didOpen` only sets contents for diagnostics, not 
> any other features.
> You would rather want to add more `# RUN:` directives at the top of the file 
> to create `compile_commands.json`, etc.
> 
> Writing it under root ('/') is obviously not an option. Lit tests allow you 
> to use temporary paths, this is probably an approach you could take. See [[ 
> https://llvm.org/docs/TestingGuide.html#substitutions | lit docs ]] for more 
> details.
Are there examples available on how to use this? I have to use a # RUN: to 
create a file and then use it's path in a workspace/didChangeConfiguration 
notification?


https://reviews.llvm.org/D39571



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

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/ClangdLSPServer.cpp:250
+  // Verify if path has value and is a valid path
+  if (Params.settings.compilationDatabasePath.hasValue()) {
+CDB.setCompileCommandsDir(

Replace `Settings` instead of `Params.settings` here?
Or remove the `Settings` variable altogether.



Comment at: clangd/ClangdServer.cpp:579
+std::vector ClangdServer::reparseOpenedFiles() {
+
+  std::promise DonePromise;

NIT: remote empty line



Comment at: clangd/ClangdServer.cpp:581
+  std::promise DonePromise;
+  std::future DoneFuture = DonePromise.get_future();
+  std::vector FutureVector;

`DonePromise` and `DoneFuture` are never used. Remove them?



Comment at: clangd/ClangdServer.cpp:587
+
+  for (auto FilePath : ActiveFilePaths)
+FutureVector.push_back(forceReparse(FilePath));

Use "const auto&" or `StringRef` instead to avoid copies?



Comment at: clangd/ClangdServer.h:244
+  /// This method is normally called when the compilation database is changed.
+
+  std::vector reparseOpenedFiles();

NIT: remove this empty line



Comment at: clangd/GlobalCompilationDatabase.cpp:108
   Logger.log("Failed to find compilation database for " + Twine(File) +
- "in overriden directory " + CompileCommandsDir.getValue() +
+ " in overriden directory " + CompileCommandsDir.getValue() +
  "\n");

Nebiroth wrote:
> ilya-biryukov wrote:
> > Accidental change?
> Twine(File) and "in overriden directory" did not have a space to separate 
> otherwise.
Right. Sorry, my mistake.



Comment at: clangd/Protocol.h:294
+/// since the data received is described as 'any' type in LSP.
+
+struct ClangdConfigurationParamsChange {

NIT: remote empty line



Comment at: clangd/Protocol.h:296
+struct ClangdConfigurationParamsChange {
+
+  llvm::Optional compilationDatabasePath;

NIT: remote empty line



Comment at: clangd/Protocol.h:303
+struct DidChangeConfigurationParams {
+
+  DidChangeConfigurationParams() {}

NIT: remove empty line



Comment at: clangd/Protocol.h:304
+
+  DidChangeConfigurationParams() {}
+  DidChangeConfigurationParams(ClangdConfigurationParamsChange settings) {}

NIT: Use `= default` instead



Comment at: clangd/Protocol.h:305
+  DidChangeConfigurationParams() {}
+  DidChangeConfigurationParams(ClangdConfigurationParamsChange settings) {}
+

We need to initialize `settings` field with `settings` parameter.

Maybe even better to remove both constructors to align with other classes in 
this file?



Comment at: test/clangd/did-change-configuration.test:15
+{"jsonrpc":"2.0","method":"workspace/didChangeConfiguration","params":{"settings":""}}
+#Failed to decode workspace/didChangeConfiguration request.
+#Incorrect mapping node

Do we want to add a `# CHECK:` for that output?



Comment at: test/clangd/did-change-configuration.test:33
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///compile_commands.json","languageId":"json","version":1,"text":"[\n{\n"directory":"/",\n"command":"/usr/bin/c++-DGTEST_HAS_RTTI=0-D_GNU_SOURCE-D__STDC_CONSTANT_MACROS-D__STDC_FORMAT_MACROS-D__STDC_LIMIT_MACROS-Ilib/Demangle-I../lib/Demangle-I/usr/include/libxml2-Iinclude-I../include-fPIC-fvisibility-inlines-hidden-Werror=date-time-std=c++11-Wall-W-Wno-unused-parameter-Wwrite-strings-Wcast-qual-Wno-missing-field-initializers-pedantic-Wno-long-long-Wno-maybe-uninitialized-Wdelete-non-virtual-dtor-Wno-comment-O0-g-fno-exceptions-fno-rtti-o/foo.c.o-c/foo.c",\n"file":"/foo.c"\n},"}}}
+

clangd won't see this file. `didOpen` only sets contents for diagnostics, not 
any other features.
You would rather want to add more `# RUN:` directives at the top of the file to 
create `compile_commands.json`, etc.

Writing it under root ('/') is obviously not an option. Lit tests allow you to 
use temporary paths, this is probably an approach you could take. See [[ 
https://llvm.org/docs/TestingGuide.html#substitutions | lit docs ]] for more 
details.



Comment at: test/clangd/initialize-params-invalid.test:2
+
 # RUN: clangd -pretty -run-synchronously < %s | FileCheck -strict-whitespace %s
 # It is absolutely vital that this file has CRLF line endings.

I am still seeing this newline


https://reviews.llvm.org/D39571



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

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[William Enright via Phabricator via cfe-commits]

Nebiroth added inline comments.



Comment at: clangd/GlobalCompilationDatabase.cpp:108
   Logger.log("Failed to find compilation database for " + Twine(File) +
- "in overriden directory " + CompileCommandsDir.getValue() +
+ " in overriden directory " + CompileCommandsDir.getValue() +
  "\n");

ilya-biryukov wrote:
> Accidental change?
Twine(File) and "in overriden directory" did not have a space to separate 
otherwise.


https://reviews.llvm.org/D39571



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

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[William Enright via Phabricator via cfe-commits]

Nebiroth updated this revision to Diff 124024.
Nebiroth marked 19 inline comments as done.
Nebiroth added a comment.

Fixed DraftStore thread-safe API being broken
Removed superfluous getCompilationDatabase call
Changed name of struct to ClangDConfigurationParamsChange
Removed operator ! overload for struct
Minor code cleanup


https://reviews.llvm.org/D39571

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/DraftStore.cpp
  clangd/DraftStore.h
  clangd/GlobalCompilationDatabase.cpp
  clangd/GlobalCompilationDatabase.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  test/clangd/did-change-configuration.test
  test/clangd/initialize-params-invalid.test
  test/clangd/initialize-params.test

Index: test/clangd/initialize-params.test
===
--- test/clangd/initialize-params.test
+++ test/clangd/initialize-params.test
@@ -18,6 +18,7 @@
 # CHECK-NEXT:  ":"
 # CHECK-NEXT:]
 # CHECK-NEXT:  },
+# CHECK-NEXT:  "configurationChangeProvider": true,
 # CHECK-NEXT:  "definitionProvider": true,
 # CHECK-NEXT:  "documentFormattingProvider": true,
 # CHECK-NEXT:  "documentOnTypeFormattingProvider": {
@@ -48,4 +49,4 @@
 # CHECK-NEXT:  "result": null
 Content-Length: 33
 
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0":"method":"exit"}
\ No newline at end of file
Index: test/clangd/initialize-params-invalid.test
===
--- test/clangd/initialize-params-invalid.test
+++ test/clangd/initialize-params-invalid.test
@@ -1,3 +1,4 @@
+
 # RUN: clangd -pretty -run-synchronously < %s | FileCheck -strict-whitespace %s
 # It is absolutely vital that this file has CRLF line endings.
 #
@@ -18,6 +19,7 @@
 # CHECK-NEXT:  ":"
 # CHECK-NEXT:]
 # CHECK-NEXT:  },
+# CHECK-NEXT:  "configurationChangeProvider": true,
 # CHECK-NEXT:  "definitionProvider": true,
 # CHECK-NEXT:  "documentFormattingProvider": true,
 # CHECK-NEXT:  "documentOnTypeFormattingProvider": {
@@ -45,4 +47,4 @@
 {"jsonrpc":"2.0","id":3,"method":"shutdown"}
 Content-Length: 33
 
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0":"method":"exit"}
\ No newline at end of file
Index: test/clangd/did-change-configuration.test
===
--- /dev/null
+++ test/clangd/did-change-configuration.test
@@ -0,0 +1,46 @@
+# RUN: clangd -run-synchronously < %s | FileCheck %s
+# It is absolutely vital that this file has CRLF line endings.
+#
+Content-Length: 125
+
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+
+Content-Length: 150
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///foo.c","languageId":"c","version":1,"text":"voidmain(){}"}}}
+
+Content-Length: 86
+
+{"jsonrpc":"2.0","method":"workspace/didChangeConfiguration","params":{"settings":""}}
+#Failed to decode workspace/didChangeConfiguration request.
+#Incorrect mapping node
+
+Content-Length: 114
+
+{"jsonrpc":"2.0","method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":{
+#Failed to decode workspace/didChangeConfiguration request.
+#compilationDatabasePath is not a scalar node
+
+Content-Length: 140
+
+{"jsonrpc":"2.0","method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":"/","testBadValue":"foo1234"}}}
+#Ignored unknown field "testBadValue"
+#Failed to find compilation database for / in overriden directory /
+#Bad field, bad compilation database
+
+Content-Length: 722
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///compile_commands.json","languageId":"json","version":1,"text":"[\n{\n"directory":"/",\n"command":"/usr/bin/c++-DGTEST_HAS_RTTI=0-D_GNU_SOURCE-D__STDC_CONSTANT_MACROS-D__STDC_FORMAT_MACROS-D__STDC_LIMIT_MACROS-Ilib/Demangle-I../lib/Demangle-I/usr/include/libxml2-Iinclude-I../include-fPIC-fvisibility-inlines-hidden-Werror=date-time-std=c++11-Wall-W-Wno-unused-parameter-Wwrite-strings-Wcast-qual-Wno-missing-field-initializers-pedantic-Wno-long-long-Wno-maybe-uninitialized-Wdelete-non-virtual-dtor-Wno-comment-O0-g-fno-exceptions-fno-rtti-o/foo.c.o-c/foo.c",\n"file":"/foo.c"\n},"}}}
+
+Content-Length: 115
+
+{"jsonrpc":"2.0","method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":"/"}}}
+#CHECK:{"jsonrpc":"2.0","method":"textDocument/publishDiagnostics","params":{"diagnostics":[{"message":"type specifier missing, defaults to 'int'","range":{"end":{"character":1,"line":0},"start":{"character":1,"line":0}},"severity":2},{"message":"control reaches end of non-void 

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added inline comments.



Comment at: clangd/Protocol.h:295
+
+struct ClangdConfigurationParams {
+

ilya-biryukov wrote:
> malaperle wrote:
> > ilya-biryukov wrote:
> > > Maybe call it `ClangdConfigurationParamsChange` to make it clear those 
> > > are diffs, not the actual params?
> > The idea was that we can reuse the same struct for 
> > InitializeParams.initializationOptions
> Since `InitializeParams.initializationOptions` may also have unset values 
> (`llvm::None`), it also seems fine to treat those as a "diff" between the 
> default parameters and the new ones.
> The reasoning behind naming for me is that if we allow only a subset of 
> fields to be set and use the ones that were set override the corresponding 
> values, it really feels like an entity describing a **change** to the 
> configuration parameters, not the parameters themselves.
> 
> I don't have a strong opinion on this one, though. If you'd prefer to keep 
> the current name, it's totally fine with me.
That makes sense the way you explained it. I think 
ClangdConfigurationParamsChange is good.


https://reviews.llvm.org/D39571



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

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/Protocol.h:295
+
+struct ClangdConfigurationParams {
+

malaperle wrote:
> ilya-biryukov wrote:
> > Maybe call it `ClangdConfigurationParamsChange` to make it clear those are 
> > diffs, not the actual params?
> The idea was that we can reuse the same struct for 
> InitializeParams.initializationOptions
Since `InitializeParams.initializationOptions` may also have unset values 
(`llvm::None`), it also seems fine to treat those as a "diff" between the 
default parameters and the new ones.
The reasoning behind naming for me is that if we allow only a subset of fields 
to be set and use the ones that were set override the corresponding values, it 
really feels like an entity describing a **change** to the configuration 
parameters, not the parameters themselves.

I don't have a strong opinion on this one, though. If you'd prefer to keep the 
current name, it's totally fine with me.


https://reviews.llvm.org/D39571



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

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added inline comments.



Comment at: clangd/Protocol.h:295
+
+struct ClangdConfigurationParams {
+

ilya-biryukov wrote:
> Maybe call it `ClangdConfigurationParamsChange` to make it clear those are 
> diffs, not the actual params?
The idea was that we can reuse the same struct for 
InitializeParams.initializationOptions


https://reviews.llvm.org/D39571



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

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/ClangdServer.cpp:580
+void ClangdServer::reparseOpenedFiles() {
+  for (auto Draft : DraftMgr.getDrafts().keys()) {
+forceReparse(Draft);

Could we have a method in `DraftStore` that returns all active files in a 
`vector` instead?
The reason is that `DraftStore` has a thread-safe API and adding `getDrafts()` 
to it totally breaks the contract.



Comment at: clangd/ClangdServer.cpp:581
+  for (auto Draft : DraftMgr.getDrafts().keys()) {
+forceReparse(Draft);
+  }

NIT: single-statement blocks don't need `{}`



Comment at: clangd/ClangdServer.h:310
 
+  /// Called when the compilation database is changed. Calls forceReparse() on
+  /// every file currently managed by DraftMgr.

NIT: description of what function does seems more useful than a particular 
use-case. Maybe swap the two sentences in the comments?

Also, `DraftMgr` is an implementation detail. Let's simply mentioned "all 
currently opened files" instead of the `DraftMgr`.
And let's add a note that this method may be really expensive.



Comment at: clangd/ClangdServer.h:312
+  /// every file currently managed by DraftMgr.
+  void reparseOpenedFiles();
+

Maybe place declaration of `reparseOpenedFiles` right after the declaration of 
`forceReparse`?
There close relation to each other is obvious, so it feels they should live 
side-by-side.



Comment at: clangd/ClangdServer.h:312
+  /// every file currently managed by DraftMgr.
+  void reparseOpenedFiles();
+

ilya-biryukov wrote:
> Maybe place declaration of `reparseOpenedFiles` right after the declaration 
> of `forceReparse`?
> There close relation to each other is obvious, so it feels they should live 
> side-by-side.
`forceReparse()` exposes a `future` to wait to its completion. We should 
probably do the same in `reparseOpenedFiles`.
The problem there is that it's probably impossible to expose using a single 
`future` with the API available in the STL (we want `when_all` for that).
Maybe return a `vector` of futures instead?



Comment at: clangd/DraftStore.h:44
+
+  const llvm::StringMap () const { return Drafts; };
+

We shouldn't expose this in the interface, as it breaks the thread-safety of 
`DraftStore`.



Comment at: clangd/GlobalCompilationDatabase.cpp:108
   Logger.log("Failed to find compilation database for " + Twine(File) +
- "in overriden directory " + CompileCommandsDir.getValue() +
+ " in overriden directory " + CompileCommandsDir.getValue() +
  "\n");

Accidental change?



Comment at: clangd/GlobalCompilationDatabase.h:57
+CompileCommandsDir = P;
+getCompilationDatabase(llvm::StringRef(P));
+CompilationDatabases.clear();

NIT: We don't need to call `StringRef` explicitly constructor here.



Comment at: clangd/GlobalCompilationDatabase.h:57
+CompileCommandsDir = P;
+getCompilationDatabase(llvm::StringRef(P));
+CompilationDatabases.clear();

ilya-biryukov wrote:
> NIT: We don't need to call `StringRef` explicitly constructor here.
Why do we need to call `getCompilationDatabase` here? Why not simply set the 
`CompileCommandsDir` and clear the `CompilationDatabases` cache?



Comment at: clangd/GlobalCompilationDatabase.h:64
   tooling::CompilationDatabase *tryLoadDatabaseFromPath(PathRef File);
+  tooling::CompilationDatabase *getCompilationDatabase(PathRef File);
 

Accidental change? Why do we need to swap the order of these functions?



Comment at: clangd/Protocol.h:295
+
+struct ClangdConfigurationParams {
+

Maybe call it `ClangdConfigurationParamsChange` to make it clear those are 
diffs, not the actual params?



Comment at: clangd/Protocol.h:300
+  parse(llvm::yaml::MappingNode *Params, clangd::Logger );
+  bool operator!() { return compilationDatabasePath.hasValue(); };
+};

Why do we overload `operator!`? Can't we use 
`llvm::Optional` where appropriate instead?



Comment at: test/clangd/initialize-params-invalid.test:1
+
 # RUN: clangd -pretty -run-synchronously < %s | FileCheck -strict-whitespace %s

Accidental newline?



Comment at: test/clangd/initialize-params-invalid.test:22
 # CHECK-NEXT:  },
+# CHECK-NEXT: "configurationChangeProvider": true,
 # CHECK-NEXT:  "definitionProvider": true,

NIT: the indent seems off by one character



Comment at: test/clangd/initialize-params.test:1
+
 # RUN: clangd -pretty -run-synchronously < %s | FileCheck -strict-whitespace %s

Accidental newline?



Comment at: 

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[William Enright via Phabricator via cfe-commits]

Nebiroth updated this revision to Diff 123069.
Nebiroth marked 3 inline comments as done.
Nebiroth added a comment.

Added test for didChangeConfiguration notification.
Compilation database and extra file flags are now properly reloaded when 
changing database path.
ClangdConfigurationParams now used instead of map.
changeConfiguration removed from ClangdServer. This class will now handle more 
specific settings.
Added more checks when parsing notification from client..


https://reviews.llvm.org/D39571

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/DraftStore.cpp
  clangd/DraftStore.h
  clangd/GlobalCompilationDatabase.cpp
  clangd/GlobalCompilationDatabase.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  test/clangd/did-change-configuration.test
  test/clangd/initialize-params-invalid.test
  test/clangd/initialize-params.test

Index: test/clangd/initialize-params.test
===
--- test/clangd/initialize-params.test
+++ test/clangd/initialize-params.test
@@ -1,3 +1,4 @@
+
 # RUN: clangd -pretty -run-synchronously < %s | FileCheck -strict-whitespace %s
 # It is absolutely vital that this file has CRLF line endings.
 #
@@ -18,6 +19,7 @@
 # CHECK-NEXT:  ":"
 # CHECK-NEXT:]
 # CHECK-NEXT:  },
+# CHECK-NEXT:	   "configurationChangeProvider": true,
 # CHECK-NEXT:  "definitionProvider": true,
 # CHECK-NEXT:  "documentFormattingProvider": true,
 # CHECK-NEXT:  "documentOnTypeFormattingProvider": {
@@ -48,4 +50,4 @@
 # CHECK-NEXT:  "result": null
 Content-Length: 33
 
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0":"method":"exit"}
\ No newline at end of file
Index: test/clangd/initialize-params-invalid.test
===
--- test/clangd/initialize-params-invalid.test
+++ test/clangd/initialize-params-invalid.test
@@ -1,3 +1,4 @@
+
 # RUN: clangd -pretty -run-synchronously < %s | FileCheck -strict-whitespace %s
 # It is absolutely vital that this file has CRLF line endings.
 #
@@ -18,6 +19,7 @@
 # CHECK-NEXT:  ":"
 # CHECK-NEXT:]
 # CHECK-NEXT:  },
+# CHECK-NEXT:	   "configurationChangeProvider": true,
 # CHECK-NEXT:  "definitionProvider": true,
 # CHECK-NEXT:  "documentFormattingProvider": true,
 # CHECK-NEXT:  "documentOnTypeFormattingProvider": {
@@ -45,4 +47,4 @@
 {"jsonrpc":"2.0","id":3,"method":"shutdown"}
 Content-Length: 33
 
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0":"method":"exit"}
\ No newline at end of file
Index: test/clangd/did-change-configuration.test
===
--- /dev/null
+++ test/clangd/did-change-configuration.test
@@ -0,0 +1,46 @@
+# RUN: clangd -run-synchronously < %s | FileCheck %s
+# It is absolutely vital that this file has CRLF line endings.
+#
+Content-Length: 125
+
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+
+Content-Length: 150
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///foo.c","languageId":"c","version":1,"text":"voidmain(){}"}}}
+
+Content-Length: 86
+
+{"jsonrpc":"2.0","method":"workspace/didChangeConfiguration","params":{"settings":""}}
+#Failed to decode workspace/didChangeConfiguration request.
+#Incorrect mapping node
+
+Content-Length: 114
+
+{"jsonrpc":"2.0","method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":{
+#Failed to decode workspace/didChangeConfiguration request.
+#compilationDatabasePath is not a scalar node
+
+Content-Length: 140
+
+{"jsonrpc":"2.0","method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":"/","testBadValue":"foo1234"}}}
+#Ignored unknown field "testBadValue"
+#Failed to find compilation database for / in overriden directory /
+#Bad field, bad compilation database
+
+Content-Length: 722
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///compile_commands.json","languageId":"json","version":1,"text":"[\n{\n"directory":"/",\n"command":"/usr/bin/c++-DGTEST_HAS_RTTI=0-D_GNU_SOURCE-D__STDC_CONSTANT_MACROS-D__STDC_FORMAT_MACROS-D__STDC_LIMIT_MACROS-Ilib/Demangle-I../lib/Demangle-I/usr/include/libxml2-Iinclude-I../include-fPIC-fvisibility-inlines-hidden-Werror=date-time-std=c++11-Wall-W-Wno-unused-parameter-Wwrite-strings-Wcast-qual-Wno-missing-field-initializers-pedantic-Wno-long-long-Wno-maybe-uninitialized-Wdelete-non-virtual-dtor-Wno-comment-O0-g-fno-exceptions-fno-rtti-o/foo.c.o-c/foo.c",\n"file":"/foo.c"\n},"}}}
+
+Content-Length: 115
+
+{"jsonrpc":"2.0","method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":"/"}}}

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[William Enright via Phabricator via cfe-commits]

Nebiroth marked 18 inline comments as done.
Nebiroth added inline comments.



Comment at: clangd/Protocol.h:285
+
+  DidChangeConfigurationParams() {}
+

malaperle wrote:
> I don't think you need this constructor?
I do inside parse() for DidChangeConfigurationParams.


https://reviews.llvm.org/D39571



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

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/ClangdServer.h:289
+  /// ChangedSettings
+  void changeConfiguration(std::map ChangedSettings);
+

Nebiroth wrote:
> ilya-biryukov wrote:
> > This function is way too general for `ClangdServer`'s interface, can we 
> > have more fine-grained settings here (i.e. `setCompletionParameters` etc?) 
> > and handle the "general" case in `ClangdLSPServer` (i.e., unknown setting 
> > names, invalid setting parameters, json parsing, etc.)?
> > 
> > I suggest we remove this function altogether.
> So if I understand correctly, we would have the most generic 
> workspace/didChangeConfiguration handler located in ClangdLSPServer
>  with a name perhaps similar to  changeConfiguration  that would pass valid 
> settings to more specific functions inside ClangdServer ?
Exactly!


https://reviews.llvm.org/D39571



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

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[William Enright via Phabricator via cfe-commits]

Nebiroth added inline comments.



Comment at: clangd/ClangdLSPServer.cpp:51
+  "definitionProvider": true,
+  "configurationChangeProvider": true
 }})");

malaperle wrote:
> Are you sure the existing tests don't fail? usually when we change this, some 
> tests have to be adjusted.
I'll verify this when I will have the time.



Comment at: clangd/ClangdServer.h:289
+  /// ChangedSettings
+  void changeConfiguration(std::map ChangedSettings);
+

ilya-biryukov wrote:
> This function is way too general for `ClangdServer`'s interface, can we have 
> more fine-grained settings here (i.e. `setCompletionParameters` etc?) and 
> handle the "general" case in `ClangdLSPServer` (i.e., unknown setting names, 
> invalid setting parameters, json parsing, etc.)?
> 
> I suggest we remove this function altogether.
So if I understand correctly, we would have the most generic 
workspace/didChangeConfiguration handler located in ClangdLSPServer
 with a name perhaps similar to  changeConfiguration  that would pass valid 
settings to more specific functions inside ClangdServer ?



Comment at: clangd/Protocol.cpp:534
 
+llvm::Optional
+DidChangeConfigurationParams::parse(llvm::yaml::MappingNode *Params,

malaperle wrote:
> I think this needs a test, perhaps create did-change-configuration.test? It 
> can also test the ClangdConfigurationParams::parse code
> If you do, I think it's a good idea to test a few failing cases:
> - "settings" is not a mapping node. You can test this with a scalar node like 
> "settings": ""
> - Something else than "settings" is present, so that it goes through 
> logIgnoredField
> - "settings" is not set at all. In this case, because it's mandatory in the 
> protocol, return llvm::None. This can be checked after the loop after all 
> key/values were read.
> 
> There are similar tests in execute-command.test if you'd like an example.
> And of course also add a "successful" case as well  :)
Yes, I was planning on adding a simple test in the coming days. I imagine this 
test will be greatly expanded on once more settings become available to change 
on the server side.


https://reviews.llvm.org/D39571



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

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov requested changes to this revision.
ilya-biryukov added a comment.

I don't think we should pass the very general configuration map to 
`ClangdServer`. Especially given that we can easily update 
`DirectoryBaseCompilationDatabase` instance in `ClangdLSPServer` itself.
What are your thoughts on this?




Comment at: clangd/ClangdLSPServer.cpp:199
+Ctx C, DidChangeConfigurationParams ) {
+  std::map SettingsMap;
+  SettingsMap.insert(std::pair(

malaperle wrote:
> I'm thinking, maybe it's better not to have the map and just pass along the 
> ClangdConfigurationParams to the server (instead of the map). I think 
> ClangdConfigurationParams is more useful as a structure than a "flat" map 
> with all the keys being at the same level. In ClangdConfigurationParams, 
> we'll be able to add sub-configurations sections (index, code-completion, 
> more?) which is well suited to reflect the JSON format.
> 
> Unless perhaps you had another use case for the map that I'm not thinking 
> about?
I don't think `ClangdServer` should handle changes to compilation database path 
at all.
It accepts `GlobalCompilationDatabase` as a parameter and does not own it, so 
`ClangdLSPServer` can mutate it properly.



Comment at: clangd/ClangdServer.h:288
+  /// Modify configuration settings based on what is contained inside
+  /// ChangedSettings
+  void changeConfiguration(std::map ChangedSettings);

NIT: Add full stop at the end of comments.



Comment at: clangd/ClangdServer.h:289
+  /// ChangedSettings
+  void changeConfiguration(std::map ChangedSettings);
+

This function is way too general for `ClangdServer`'s interface, can we have 
more fine-grained settings here (i.e. `setCompletionParameters` etc?) and 
handle the "general" case in `ClangdLSPServer` (i.e., unknown setting names, 
invalid setting parameters, json parsing, etc.)?

I suggest we remove this function altogether.


https://reviews.llvm.org/D39571



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

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle requested changes to this revision.
malaperle added inline comments.
This revision now requires changes to proceed.



Comment at: clangd/ClangdLSPServer.cpp:51
+  "definitionProvider": true,
+  "configurationChangeProvider": true
 }})");

Are you sure the existing tests don't fail? usually when we change this, some 
tests have to be adjusted.



Comment at: clangd/ClangdLSPServer.cpp:199
+Ctx C, DidChangeConfigurationParams ) {
+  std::map SettingsMap;
+  SettingsMap.insert(std::pair(

I'm thinking, maybe it's better not to have the map and just pass along the 
ClangdConfigurationParams to the server (instead of the map). I think 
ClangdConfigurationParams is more useful as a structure than a "flat" map with 
all the keys being at the same level. In ClangdConfigurationParams, we'll be 
able to add sub-configurations sections (index, code-completion, more?) which 
is well suited to reflect the JSON format.

Unless perhaps you had another use case for the map that I'm not thinking about?



Comment at: clangd/ClangdLSPServer.cpp:201
+  SettingsMap.insert(std::pair(
+  "CDBPath", Params.settings.compilationDatabasePath.getValue()));
+  
CDB.setCompileCommandsDir(Params.settings.compilationDatabasePath.getValue());

Won't getValue crash if compilationDatabasePath was not set since it's optional?



Comment at: clangd/ClangdLSPServer.cpp:203
+  
CDB.setCompileCommandsDir(Params.settings.compilationDatabasePath.getValue());
+  
CDB.getCompilationDatabase(StringRef(CDB.getCompileCommandsDir().getValue()));
+

Why is this line needed? Maybe there should be a comment?
I think its meant to reload the actual CompilationDatabase object? I think you 
could call this in setCompileCommandsDir and not have to make 
getCompilationDatabase public. You also woudn't need getCompileCommandsDir at 
all.



Comment at: clangd/ClangdLSPServer.cpp:208
+  // map and sent with this function call.
+  Server.changeConfiguration(SettingsMap);
+}

Pass Params.settings? see previous comment.



Comment at: clangd/ClangdServer.cpp:441
+std::map ChangedSettings) {
+  if (!ChangedSettings.empty()) {
+  }

I think we need to discuss what should happen when the compilation database 
changes. Since this is mainly for switching "configuration", I think it should 
invalidate all previously parsed units. Otherwise, if the user has a file open 
in the editor, it will report diagnostics based on the old configuration and 
similarly code completion will be outdated (until Clangd is restarted?). Would 
it be unreasonable to reparse all units in memory? Perhaps 
ClangdServer::forceReparse would be useful for that?
I'm not sure what is the right approach but we should make sure the editors are 
not left in a confusing state.



Comment at: clangd/GlobalCompilationDatabase.h:55
   getCompileCommands(PathRef File) override;
+  llvm::Optional getCompileCommandsDir() { return CompileCommandsDir; }
+  void setCompileCommandsDir(Path P) { CompileCommandsDir = P; }

remove?



Comment at: clangd/GlobalCompilationDatabase.h:56
+  llvm::Optional getCompileCommandsDir() { return CompileCommandsDir; }
+  void setCompileCommandsDir(Path P) { CompileCommandsDir = P; }
+

call getCompilationDatabase from here?



Comment at: clangd/GlobalCompilationDatabase.h:58
+
+  tooling::CompilationDatabase *getCompilationDatabase(PathRef File);
 

move back to private?



Comment at: clangd/Protocol.cpp:534
 
+llvm::Optional
+DidChangeConfigurationParams::parse(llvm::yaml::MappingNode *Params,

I think this needs a test, perhaps create did-change-configuration.test? It can 
also test the ClangdConfigurationParams::parse code
If you do, I think it's a good idea to test a few failing cases:
- "settings" is not a mapping node. You can test this with a scalar node like 
"settings": ""
- Something else than "settings" is present, so that it goes through 
logIgnoredField
- "settings" is not set at all. In this case, because it's mandatory in the 
protocol, return llvm::None. This can be checked after the loop after all 
key/values were read.

There are similar tests in execute-command.test if you'd like an example.
And of course also add a "successful" case as well  :)



Comment at: clangd/Protocol.cpp:561
+
+  return Result;
+}

If "settings" was not set in the end, return llvm::None, because this is 
mandatory in the protocol.



Comment at: clangd/Protocol.cpp:578
+if (!Value)
+  return llvm::None;
+

Here there should be a test when 

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[William Enright via Phabricator via cfe-commits]

Nebiroth created this revision.

Implementation of DidChangeConfiguration notification handling in clangd.
This currently only supports changing one setting: the path of the compilation 
database to be used for the current project
In other words, it is no longer necessary to restart clangd with a different 
command line argument in order to change
the compilation database.


https://reviews.llvm.org/D39571

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/GlobalCompilationDatabase.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h

Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -52,6 +52,8 @@
   virtual void onGoToDefinition(Ctx C, TextDocumentPositionParams ) = 0;
   virtual void onSwitchSourceHeader(Ctx C, TextDocumentIdentifier ) = 0;
   virtual void onFileEvent(Ctx C, DidChangeWatchedFilesParams ) = 0;
+  virtual void onChangeConfiguration(Ctx C,
+ DidChangeConfigurationParams ) = 0;
 };
 
 void registerCallbackHandlers(JSONRPCDispatcher , JSONOutput ,
Index: clangd/ProtocolHandlers.cpp
===
--- clangd/ProtocolHandlers.cpp
+++ clangd/ProtocolHandlers.cpp
@@ -72,4 +72,6 @@
   Register("textDocument/switchSourceHeader",
::onSwitchSourceHeader);
   Register("workspace/didChangeWatchedFiles", ::onFileEvent);
+  Register("workspace/didChangeConfiguration",
+   ::onChangeConfiguration);
 }
Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -273,6 +273,27 @@
   parse(llvm::yaml::MappingNode *Params, clangd::Logger );
 };
 
+struct ClangdConfigurationParams {
+
+  llvm::Optional compilationDatabasePath;
+  static llvm::Optional
+  parse(llvm::yaml::MappingNode *Params, clangd::Logger );
+};
+
+struct DidChangeConfigurationParams {
+
+  DidChangeConfigurationParams() {}
+
+  DidChangeConfigurationParams(ClangdConfigurationParams settings)
+  : settings(settings) {}
+
+  ClangdConfigurationParams settings;
+
+  static llvm::Optional
+  parse(llvm::yaml::MappingNode *Params, clangd::Logger );
+  static std::string unparse(const DidChangeConfigurationParams );
+};
+
 struct FormattingOptions {
   /// Size of a tab in spaces.
   int tabSize;
Index: clangd/Protocol.cpp
===
--- clangd/Protocol.cpp
+++ clangd/Protocol.cpp
@@ -531,6 +531,59 @@
   return Result;
 }
 
+llvm::Optional
+DidChangeConfigurationParams::parse(llvm::yaml::MappingNode *Params,
+clangd::Logger ) {
+  DidChangeConfigurationParams Result;
+  for (auto  : *Params) {
+auto *KeyString = dyn_cast(NextKeyValue.getKey());
+if (!KeyString)
+  return llvm::None;
+
+llvm::SmallString<10> KeyStorage;
+StringRef KeyValue = KeyString->getValue(KeyStorage);
+auto *Value =
+dyn_cast_or_null(NextKeyValue.getValue());
+if (!Value)
+  return llvm::None;
+
+llvm::SmallString<10> Storage;
+if (KeyValue == "settings") {
+  auto Parsed = ClangdConfigurationParams::parse(Value, Logger);
+  if (!Parsed)
+return llvm::None;
+  Result.settings = Parsed.getValue();
+} else {
+  logIgnoredField(KeyValue, Logger);
+}
+  }
+
+  return Result;
+}
+
+llvm::Optional
+ClangdConfigurationParams::parse(llvm::yaml::MappingNode *Params,
+ clangd::Logger ) {
+  ClangdConfigurationParams Result;
+  for (auto  : *Params) {
+auto *KeyString = dyn_cast(NextKeyValue.getKey());
+if (!KeyString)
+  return llvm::None;
+
+llvm::SmallString<10> KeyStorage;
+StringRef KeyValue = KeyString->getValue(KeyStorage);
+auto *Value =
+dyn_cast_or_null(NextKeyValue.getValue());
+if (!Value)
+  return llvm::None;
+
+if (KeyValue == "compilationDatabasePath") {
+  Result.compilationDatabasePath = Value->getValue(KeyStorage);
+}
+  }
+  return Result;
+}
+
 llvm::Optional
 TextDocumentContentChangeEvent::parse(llvm::yaml::MappingNode *Params,
   clangd::Logger ) {
Index: clangd/GlobalCompilationDatabase.h
===
--- clangd/GlobalCompilationDatabase.h
+++ clangd/GlobalCompilationDatabase.h
@@ -52,11 +52,14 @@
 
   std::vector
   getCompileCommands(PathRef File) override;
+  llvm::Optional getCompileCommandsDir() { return CompileCommandsDir; }
+  void setCompileCommandsDir(Path P) { CompileCommandsDir = P; }
+
+  tooling::CompilationDatabase *getCompilationDatabase(PathRef File);
 
   void setExtraFlagsForFile(PathRef File, std::vector ExtraFlags);
 
 private:
-  tooling::CompilationDatabase