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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/ClangdLSPServer.h:23
+
+class ClangdLSPServer {
+  class LSPDiagnosticsConsumer;
----------------
klimek wrote:
> 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.
Added comments to this class. And ClangdServer, it didn't have a comment too.


================
Comment at: clangd/ClangdUnit.h:43
+
+  /// Reparse with new contents
+  void reparse(StringRef Contents);
----------------
klimek wrote:
> Tiny nit: generally, use "." at the end of sentences :)
Fixed


================
Comment at: clangd/GlobalCompilationDatabase.h:28
+/// Provides compilation arguments used for building ClangdUnit.
+class GlobalCompilationDatabase {
+public:
----------------
klimek wrote:
> 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?
It has getAllFiles and getAllCompileCommands which we don't need for now.
It doesn't have capabilities to track changes to flags, which we'll want to add at some point. (Added a comment that we'll be adding those).

Also, currently implementations of CompilationDatabase actually require us to recreate them for each file, this logic is currently moved into DirectoryBasedGlobalCompilationDatabase for convenience.


https://reviews.llvm.org/D33047