[PATCH] D33047: [ClangD] Refactor clangd into separate components

[Manuel Klimek via Phabricator via cfe-commits]

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