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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
