klimek added inline comments.
================
Comment at: clangd/ClangdLSPServer.h:23
+
+class ClangdLSPServer {
+ class LSPDiagnosticsConsumer;
----------------
I'd have expected something that's called LSP server to work on the LSP
protocol level (that is, have a server(iostream) equivalent method).
================
Comment at: clangd/ClangdLSPServer.h:24-25
+class ClangdLSPServer {
+ class LSPDiagnosticsConsumer;
+
+public:
----------------
I like to put the public interface first, as that's what people usually look at
first.
================
Comment at: clangd/ClangdLSPServer.h:48
+
+private:
+ JSONOutput &Out;
----------------
AFAIK usually we don't duplicate access modifiers.
================
Comment at: clangd/ClangdUnit.h:44
+ /// Reparse with new contents
+ void reparse(StringRef Contents);
+
----------------
One question is how we can handle when we want the old one to stick around
while we do a full reparse (when #includes change), so we still have fast code
completion.
================
Comment at: clangd/ClangdUnitStore.h:21
+namespace clang {
+namespace clangd {
+/// Thread-safe collection of ASTs built for specific files. Provides
----------------
Tiny nit: please add empty lines between classes and namespace delcs :)
================
Comment at: clangd/ClangdUnitStore.h:29-30
+ void runOnUnit(PathRef File, StringRef FileContents,
+ GlobalCompilationDatabase &CDB,
+ std::shared_ptr<PCHContainerOperations> PCHs, Func Action,
+ bool ReparseBeforeAction = true) {
----------------
Not having read enough of the rest of the patch yet, I'd expect these things to
be stored on the class level instead of being passed around all the time.
Similarly, we're missing the VFS?
================
Comment at: clangd/ClangdUnitStore.h:31
+ std::shared_ptr<PCHContainerOperations> PCHs, Func Action,
+ bool ReparseBeforeAction = true) {
+ std::lock_guard<std::mutex> Lock(Mutex);
----------------
Parameters after a function are unfortunate, as they make the code flow weird
when you want to pass in a lambda, but still want to set the parameter after it
:(
Given how different the semantics are, I'd perhaps make these 2 functions:
// Run on the translation unit.
// If there was a previous call of runOnUnitWithContent, the content provided
there will be used.
// Otherwise, the file will be read from VFS.
runOnStoredUnit(PathRef File, Func Action);
// Run on the translation unit with the given content.
// Content will be stored as in-flight code for the given translation unit
only:
// - if a different translation unit is parsed that #includes File, Content
will not be used;
// - subsequent calls to runOnStoredUnit will re-use Content.
runOnUnitWithContent(PathRef File, StringRef Content, Func Action);
================
Comment at: clangd/ClangdUnitStore.h:32
+ bool ReparseBeforeAction = true) {
+ std::lock_guard<std::mutex> Lock(Mutex);
+
----------------
This is due to a restriction in the AST unit? If we don't need to reparse, why
can't we run multiple things over an AST at once?
================
Comment at: clangd/GlobalCompilationDatabase.cpp:55
+
+ // TODO(ibiryukov): Invalidate cached compilation databases on changes
+ auto result = CDB.get();
----------------
Here and elsewhere: in Clang, we use FIXME instead of TODO :)
================
Comment at: clangd/GlobalCompilationDatabase.h:28
+/// Provides compilation arguments used for building ClangdUnit.
+class GlobalCompilationDatabase {
+public:
----------------
What's global about this?
https://reviews.llvm.org/D33047
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
