[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<Path>` 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<VersionedDraft> &getDrafts() 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 &Logger);
+  bool operator!() { return compilationDatabasePath.hasValue(); };
+};
----------------
Why do we overload `operator!`? Can't we use `llvm::Optional<ClangdConfigurationParams>` 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: test/clangd/initialize-params.test:22
 # CHECK-NEXT:      },
+# CHECK-NEXT:	   "configurationChangeProvider": true,
 # CHECK-NEXT:      "definitionProvider": true,
----------------
NIT: the indent seems off by one character



https://reviews.llvm.org/D39571