[PATCH] D39571: [clangd] DidChangeConfiguration Notification
Ilya Biryukov via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Nov 16 05:09:26 PST 2017
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
More information about the cfe-commits
mailing list