From 0d9b40f583f49cfb7deb4bd734f9c3e3160413c5 Mon Sep 17 00:00:00 2001 From: Sam McCall Date: Fri, 19 Oct 2018 15:42:23 +0000 Subject: [PATCH] [clangd] Set workspace root when initializing ClangdServer, disallow mutation. Summary: Rename instance variable to WorkspaceRoot to match what we call it internally. Add fixme to set it automatically. Don't do it yet, clients have assumptions that the constructor won't access the FS. Don't second-guess the provided root. Reviewers: ioeric Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, cfe-commits Differential Revision: https://reviews.llvm.org/D53404 llvm-svn: 344787 --- clang-tools-extra/clangd/ClangdLSPServer.cpp | 9 ++++----- clang-tools-extra/clangd/ClangdServer.cpp | 15 ++------------- clang-tools-extra/clangd/ClangdServer.h | 11 ++++++----- .../unittests/clangd/FindSymbolsTests.cpp | 2 +- 4 files changed, 13 insertions(+), 24 deletions(-) diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp index 46edf5732dd9..cf964c14e306 100644 --- a/clang-tools-extra/clangd/ClangdLSPServer.cpp +++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp @@ -261,6 +261,10 @@ void ClangdLSPServer::reply(llvm::json::Value ID, void ClangdLSPServer::onInitialize(const InitializeParams &Params, Callback Reply) { + if (Params.rootUri && *Params.rootUri) + ClangdServerOpts.WorkspaceRoot = Params.rootUri->file(); + else if (Params.rootPath && !Params.rootPath->empty()) + ClangdServerOpts.WorkspaceRoot = *Params.rootPath; if (Server) return Reply(make_error("server already initialized", ErrorCode::InvalidRequest)); @@ -277,11 +281,6 @@ void ClangdLSPServer::onInitialize(const InitializeParams &Params, applyConfiguration(Opts.ParamsChange); } - if (Params.rootUri && *Params.rootUri) - Server->setRootPath(Params.rootUri->file()); - else if (Params.rootPath && !Params.rootPath->empty()) - Server->setRootPath(*Params.rootPath); - CCOpts.EnableSnippets = Params.capabilities.CompletionSnippets; DiagOpts.EmbedFixesInDiagnostics = Params.capabilities.DiagnosticFixes; DiagOpts.SendDiagnosticCategory = Params.capabilities.DiagnosticCategory; diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp index 577222c20b65..b0b1af1bbe48 100644 --- a/clang-tools-extra/clangd/ClangdServer.cpp +++ b/clang-tools-extra/clangd/ClangdServer.cpp @@ -109,6 +109,7 @@ ClangdServer::ClangdServer(const GlobalCompilationDatabase &CDB, ? new FileIndex(Opts.URISchemes, Opts.HeavyweightDynamicSymbolIndex) : nullptr), + WorkspaceRoot(Opts.WorkspaceRoot), PCHs(std::make_shared()), // Pass a callback into `WorkScheduler` to extract symbols from a newly // parsed file and rebuild the file index synchronously each time an AST @@ -131,18 +132,6 @@ ClangdServer::ClangdServer(const GlobalCompilationDatabase &CDB, Index = nullptr; } -void ClangdServer::setRootPath(PathRef RootPath) { - auto FS = FSProvider.getFileSystem(); - auto Status = FS->status(RootPath); - if (!Status) - elog("Failed to get status for RootPath {0}: {1}", RootPath, - Status.getError().message()); - else if (Status->isDirectory()) - this->RootPath = RootPath; - else - elog("The provided RootPath {0} is not a directory.", RootPath); -} - void ClangdServer::addDocument(PathRef File, StringRef Contents, WantDiagnostics WantDiags) { DocVersion Version = ++InternalVersion[File]; @@ -495,7 +484,7 @@ void ClangdServer::onFileEvent(const DidChangeWatchedFilesParams &Params) { void ClangdServer::workspaceSymbols( StringRef Query, int Limit, Callback> CB) { CB(clangd::getWorkspaceSymbols(Query, Limit, Index, - RootPath ? *RootPath : "")); + WorkspaceRoot.getValueOr(""))); } void ClangdServer::documentSymbols( diff --git a/clang-tools-extra/clangd/ClangdServer.h b/clang-tools-extra/clangd/ClangdServer.h index 400e464a17bb..be508653daef 100644 --- a/clang-tools-extra/clangd/ClangdServer.h +++ b/clang-tools-extra/clangd/ClangdServer.h @@ -86,6 +86,11 @@ public: /// If set, use this index to augment code completion results. SymbolIndex *StaticIndex = nullptr; + /// Clangd's workspace root. Relevant for "workspace" operations not bound + /// to a particular file. + /// FIXME: If not set, should use the current working directory. + llvm::Optional WorkspaceRoot; + /// The resource directory is used to find internal headers, overriding /// defaults and -resource-dir compiler flag). /// If None, ClangdServer calls CompilerInvocation::GetResourcePath() to @@ -116,9 +121,6 @@ public: const FileSystemProvider &FSProvider, DiagnosticsConsumer &DiagConsumer, const Options &Opts); - /// Set the root path of the workspace. - void setRootPath(PathRef RootPath); - /// Add a \p File to the list of tracked C++ files or update the contents if /// \p File is already tracked. Also schedules parsing of the AST for it on a /// separate thread. When the parsing is complete, DiagConsumer passed in @@ -255,8 +257,7 @@ private: CachedCompletionFuzzyFindRequestByFile; mutable std::mutex CachedCompletionFuzzyFindRequestMutex; - // If set, this represents the workspace path. - llvm::Optional RootPath; + llvm::Optional WorkspaceRoot; std::shared_ptr PCHs; /// Used to serialize diagnostic callbacks. /// FIXME(ibiryukov): get rid of an extra map and put all version counters diff --git a/clang-tools-extra/unittests/clangd/FindSymbolsTests.cpp b/clang-tools-extra/unittests/clangd/FindSymbolsTests.cpp index 068660d77aee..744129d8a252 100644 --- a/clang-tools-extra/unittests/clangd/FindSymbolsTests.cpp +++ b/clang-tools-extra/unittests/clangd/FindSymbolsTests.cpp @@ -42,6 +42,7 @@ MATCHER_P(SymRange, Range, "") { return arg.location.range == Range; } ClangdServer::Options optsForTests() { auto ServerOpts = ClangdServer::optsForTest(); + ServerOpts.WorkspaceRoot = testRoot(); ServerOpts.BuildDynamicSymbolIndex = true; ServerOpts.URISchemes = {"unittest", "file"}; return ServerOpts; @@ -53,7 +54,6 @@ public: : Server(CDB, FSProvider, DiagConsumer, optsForTests()) { // Make sure the test root directory is created. FSProvider.Files[testPath("unused")] = ""; - Server.setRootPath(testRoot()); } protected: