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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
