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