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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/ClangdLSPServer.h:132
 
-  RealFileSystemProvider FSProvider;
   /// Options used for code completion
----------------
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.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55256