[PATCH] D55256: [clangd] Support clang-tidy configuration in clangd.

[Haojian Wu via Phabricator via cfe-commits]

hokein marked an inline comment as done.
hokein added inline comments.


================
Comment at: clangd/ClangdLSPServer.h:132
 
-  RealFileSystemProvider FSProvider;
   /// Options used for code completion
----------------
ilya-biryukov wrote:
> hokein wrote:
> > sammccall wrote:
> > > ilya-biryukov wrote:
> > > > Could we instead call `getRealFS()` when we try to initialize a clang-tidy options provider in `main()` and avoid changing this?
> > > > To avoid adding extra non-real-fs "modes of operation" to `ClangdLSPServer`. Unless you see other uses for this.
> > > We already have out-of-tree modifications to ClangdLSPServer to use non-real FSes.
> > > Given that, I think this change is OK... though better still might be to move it into `ClangdServer::Options`
> > Yes, this is the main reason I did this change.
> It still feels that `ClangdLSPServcer` is closely tied to real fs, so I don't see how that makes things simpler. I wouldn't call the presence of out-of-tree modifications a good reason to do this change, at least not without tests and comments explaining why this needs to be configurable.
> 
> WRT to `ClangdServer::Options`, I believe this goes back to the previous discussion we had about putting the non-data configuration parameters (Index+FSProvider+CompilationsDB). I'd say put `CompilationsDB` into `Options` as well if you plan to put FSProvider, they should really live together (i.e. the reason we have out-of-tree modifications is closely tied to our custom CDB, so it makes sense for both to stay together). 
> 
> FWIW, I still think they're FSProvider+CDB+Index should be separate parameters, but that goes back to the earlier conversation we had with @sammccall when the `Options` were added in the first place.
> 
> Not a big deal, just wanted to convey the intention of my original comment.
> It still feels that ClangdLSPServcer is closely tied to real fs, so I don't see how that makes things simpler. I wouldn't call the presence of out-of-tree modifications a good reason to do this change, at least not without tests and comments explaining why this needs to be configurable.

This would simplifier our internal modification (just 1-2 line changes) -- ClangTidyOptionProvider needs to know FileSystem, we need a way to retrieve it (I believe both clangdLSPServer and ClangTidyOptionProvider should share the same non-real FS internally). An optional way is to add a `GetFS` method in ClangdLSPServer.

Anyway, I think this shouldn't block this patch, we could revisit it afterwards.




Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55256/new/

https://reviews.llvm.org/D55256