klimek added inline comments.
================
Comment at: clangd/ClangdLSPServer.h:23
+
+class ClangdLSPServer {
+ class LSPDiagnosticsConsumer;
----------------
ilya-biryukov wrote:
> klimek wrote:
> > 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).
> The idea is to have a class that wraps ClangdServer and is called by clangd's
> ProtocolHandlers.cpp to do all the work.
> I.e. it's a replacement of what used to be called 'ASTManager'. Any
> suggestions for a better name? ClangdLSPHandler?
>
> It's seems hard to implement the interface you suggest right away, lots of
> code will have to be moved. Happy to do that in further commits, but suggest
> we leave this class as is in this commit.
I think it's fine to have an intermediate step checked in; the important part
is that we add comments explaining why this is the way it is, especially if we
plan to change it in the future :) -> I'd add a class level comment explaining
what this class does and why it exists.
================
Comment at: clangd/ClangdUnit.h:43
+
+ /// Reparse with new contents
+ void reparse(StringRef Contents);
----------------
Tiny nit: generally, use "." at the end of sentences :)
================
Comment at: clangd/GlobalCompilationDatabase.h:28
+/// Provides compilation arguments used for building ClangdUnit.
+class GlobalCompilationDatabase {
+public:
----------------
ilya-biryukov wrote:
> klimek wrote:
> > What's global about this?
> The fact that it's created only once(as opposed to
> tooling::CompilationDatabase, which can be recreated for each file) and
> handles all CompileCommand-related things(i.e. the idea is to add tracking of
> compile flags changes to this interface).
> I called it ClangdCompilationDatabase first, but then I thought there's too
> much Clangd-prefixed names :-)
> Any suggestions for a better name?
Why don't we just use a tooling::CompilationDatabase and pass it around?
https://reviews.llvm.org/D33047
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
