[PATCH] D44088: [clangd] Extract ClangdServer::Options struct.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/ClangdServer.h:94
+  /// Returns a statically allocated instance. (Stateless, safe to share).
+  static RealFileSystemProvider *get();
+
----------------
Maybe return a reference instead of a pointer?


================
Comment at: clangd/ClangdServer.h:107
 public:
-  /// Creates a new ClangdServer instance.
-  /// To process parsing requests asynchronously, ClangdServer will spawn \p
-  /// AsyncThreadsCount worker threads. However, if \p AsyncThreadsCount is 0,
-  /// all requests will be processed on the calling thread.
   ///
+  struct Options {
----------------
NIT: remove empty comment?


================
Comment at: clangd/ClangdServer.h:136
+    /// returns along with the vfs::FileSystem.
+    FileSystemProvider *FSProvider = RealFileSystemProvider::get();
+  };
----------------
Could we keep FSProvider a separate parameter?

- Having it as a reference clearly states that it can't be null on the type level.
- It is a C++  extension point, rather than a configuration parameter. I'd vouch for not mixing those together in the same struct.

We could group GlobalCompilationDatabase, DiagnosticsConsumer, FSProvider into a separate struct if the number of parameters is a concern. I'd put StaticIndex there as well, even though that would mean two index-related options are separated.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44088