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

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

In https://reviews.llvm.org/D50446#1192282, @ilya-biryukov wrote:

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


Thanks for the summary! Just for the record, my concerns were:

1. The concept of active files are not really index-specific.
2. We need to do this for all 3 different requests (need "scary" comments 3 places?).
3. The context value seems to be trivial enough and hard to be misused.

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



================
Comment at: clangd/ClangdServer.cpp:162
 
+    WithContextValue WithFileName(kActiveFile, File);
     // FIXME(ibiryukov): even if Preamble is non-null, we may want to check
----------------
ilya-biryukov wrote:
> 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.
I have two concerns about doing this:
1. Setting it in the TUScheduler seems to bury this deeper e.g. into scheduling logic. Limiting this in specific callbacks right before it's needed seems to make it less accessible.

2. There are operations like `workspaceSymbols` (although not handled in this patch)  that do not go through TUScheduler.


================
Comment at: clangd/Context.cpp:35
 
+const clang::clangd::Key<llvm::StringRef> kActiveFile;
+
----------------
ilya-biryukov wrote:
> 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.
I intentionally used `StringRef` here to make it harder to use and hoped that users would be more careful when using this. For the current simple use cases, `StringRef` should be sufficient IMO.


================
Comment at: clangd/Context.cpp:35
 
+const clang::clangd::Key<llvm::StringRef> kActiveFile;
+
----------------
ioeric wrote:
> ilya-biryukov wrote:
> > 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.
> I intentionally used `StringRef` here to make it harder to use and hoped that users would be more careful when using this. For the current simple use cases, `StringRef` should be sufficient IMO.
We should probably look for some other place to put this, but I don't think `Index.h` is the right place. Although context might not be the best place to host the active file, the active file seems to be a well-defined concept in clangd in general and not index-specific.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50446