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

Ilya Biryukov via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Mar 5 06:21:35 PST 2018

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
-  /// 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.

  rCTE Clang Tools Extra


More information about the cfe-commits mailing list