[PATCH] D50446: [clangd] Record the currently active file in context for codeCompletion and findDefinitions.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Short summary of the offline discussion: I suggest adding this parameter into the corresponding requests of the index (FuzzyFindRequest, FindDefinitionsRequest) instead of stashing it in the context. Context has all the same problems as the global variables, so we should try to avoid using it where possible.
On the other hand, where this key is stored might not be terribly important if we don't have too many usages and remove it when we can workaround the limitations that we're currently facing.

In any case, we should add a comment that the active file should be avoided and will be removed.



================
Comment at: clangd/ClangdServer.cpp:162
 
+    WithContextValue WithFileName(kActiveFile, File);
     // FIXME(ibiryukov): even if Preamble is non-null, we may want to check
----------------
If we want to set this value more consistently, maybe do it in TUScheduler::runWithPreamble/runWithAST? 
All interesting operations that work on files go through those, so it's easier to certify that the file is set correctly.


================
Comment at: clangd/Context.cpp:35
 
+const clang::clangd::Key<llvm::StringRef> kActiveFile;
+
----------------
Maybe move it somewhere else? E.g. to `Index.h`, since that's where it's supposed to be used to workaround current limitations?
`Context.h/cpp` is a general-purpose support library for the contexts, application-specific keys do not fit in well there.


================
Comment at: clangd/Context.cpp:35
 
+const clang::clangd::Key<llvm::StringRef> kActiveFile;
+
----------------
ilya-biryukov wrote:
> Maybe move it somewhere else? E.g. to `Index.h`, since that's where it's supposed to be used to workaround current limitations?
> `Context.h/cpp` is a general-purpose support library for the contexts, application-specific keys do not fit in well there.
Maybe use `std::string`? Contexts can easily be cloned/migrated between threads, so it's very easy to end up with pointers to dead strings here.


================
Comment at: clangd/Context.h:223
+// individual requests.
+extern const clang::clangd::Key<llvm::StringRef> kActiveFile;
+
----------------
Maybe choose a scarier name and add a comment that we're not gonna use it in the long-term?
Maybe also add a comment that this is only used in our index implementation to workaround the limitations of the current design? 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50446