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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits