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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/ClangdServer.cpp:162
 
+    WithContextValue WithFileName(kActiveFile, File);
     // FIXME(ibiryukov): even if Preamble is non-null, we may want to check
----------------
ioeric wrote:
> 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.
> 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.

If we want to make the data-flow explicit, let's add this to the index request parameters.
Otherwise, I suggest we do something consistent across all requests, rather than special-casing the index-calling ones. E.g. if someone adds an index call in `onHover`, they'll need to remember to also update the ClangdServer call to set this value and they really shouldn't care about this hack of ours. The less intrusive this thing is, the better.

> 2. There are operations like workspaceSymbols (although not handled in this patch) that do not go through TUScheduler.
And we don't set the file path for them anyway, because there's not active file in that case. How is that different in either of the approaches?


================
Comment at: clangd/Context.cpp:35
 
+const clang::clangd::Key<llvm::StringRef> kActiveFile;
+
----------------
ioeric wrote:
> 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.
Maybe move it `ClangdServer.h` or `TUScheduler,h` then? (depending on where we set it).

I still don't think `Context.h` is the right place for it,  it should not contain any application-specific keys.


================
Comment at: clangd/Context.cpp:35
 
+const clang::clangd::Key<llvm::StringRef> kActiveFile;
+
----------------
ilya-biryukov wrote:
> ioeric wrote:
> > 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.
> Maybe move it `ClangdServer.h` or `TUScheduler,h` then? (depending on where we set it).
> 
> I still don't think `Context.h` is the right place for it,  it should not contain any application-specific keys.
We're relying too much on the data flow of contexts, which is implicit and we completely don't control. Even if we don't access dead StringRefs now, it's very easy to accidentally introduce this behavior later.

We should use `string` here, there's just too high risk of hitting a bug otherwise. What are the disadvantages of using `string` here?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50446