kuganv added inline comments.
================
Comment at: clang-tools-extra/clangd/Preamble.cpp:694
Result->MainIsIncludeGuarded = CapturedInfo.isMainFileIncludeGuarded();
- return Result;
+ CapturedCtx.emplace(CapturedInfo.takeLife());
+ return std::make_pair(Result, CapturedCtx);
----------------
kadircet wrote:
> kuganv wrote:
> > kadircet wrote:
> > > what about just keeping the callback (with a different signature) and
> > > calling it here? e.g.:
> > > ```
> > > PreambleCallback(CapturedInfo.takeLife());
> > > ```
> > >
> > > that way we don't need to change the return type and also control if we
> > > received a valid object on every call site (since callback will only be
> > > invoked on success)
> > > what about just keeping the callback (with a different signature) and
> > > calling it here? e.g.:
> > > ```
> > > PreambleCallback(CapturedInfo.takeLife());
> > > ```
> > >
> > > that way we don't need to change the return type and also control if we
> > > received a valid object on every call site (since callback will only be
> > > invoked on success)
> >
> > Apologies for the misunderstanding. Just to be clear, you prefer indexing
> > in UpdateIndexCallbacks using the thread Tasks that also indexes in
> > indexStdlib? In this implementation, I am calling the index action in
> > preamble thread. I will revise it.
> > Apologies for the misunderstanding. Just to be clear, you prefer indexing
> > in UpdateIndexCallbacks using the thread Tasks that also indexes in
> > indexStdlib? In this implementation, I am calling the index action in
> > preamble thread. I will revise it.
>
> Yes, i guess that's the reason why I was confused while looking at the code.
> Sorry if I give the impression that suggests doing the indexing on
> `PreambleThread`, but I think both in my initial comment:
>
> >> As for AsyncTaskRunner to use, since this indexing task only depends on
> >> the file-index, which is owned by ClangdServer, I don't think there's any
> >> need to introduce a new taskrunner into TUScheduler and block its
> >> destruction. We can just re-use the existing TaskRunner inside
> >> parsingcallbacks, in which we run stdlib indexing tasks.
>
> and in the follow up;
>
> >> I think we can just change the signature for PreambleParsedCallback to
> >> pass along refcounted objects. forgot to mention in the first comment, but
> >> we should also change the CanonicalIncludes to be a shared_ptr so that it
> >> can outlive the PreambleData. We should invoke the callback inside
> >> buildPreamble after a successful build. Afterwards we should also change
> >> the signature for onPreambleAST to take AST, PP and CanonicalIncludes as
> >> ref-counted objects again and PreambleThread::build should just forward
> >> objects received from PreambleParsedCallback. Afterwards inside the
> >> UpdateIndexCallbacks::onPreambleAST we can just invoke indexing on Tasks
> >> if it's present or synchronously in the absence of it.
>
> I was pointing towards running this inside the `Tasks` in
> `UpdateIndexCallbacks`.
>
> ---
>
> There's definitely some upsides to running that indexing on the preamble
> thread as well (which is what we do today) but I think the extra sequencing
> requirements (make sure to first notify the ASTPeer and then issue preamble
> callbacks) we put into TUScheduler (which is already quite complex) is
> probably not worth it.
> > Apologies for the misunderstanding. Just to be clear, you prefer indexing
> > in UpdateIndexCallbacks using the thread Tasks that also indexes in
> > indexStdlib? In this implementation, I am calling the index action in
> > preamble thread. I will revise it.
>
> Yes, i guess that's the reason why I was confused while looking at the code.
> Sorry if I give the impression that suggests doing the indexing on
> `PreambleThread`, but I think both in my initial comment:
>
> >> As for AsyncTaskRunner to use, since this indexing task only depends on
> >> the file-index, which is owned by ClangdServer, I don't think there's any
> >> need to introduce a new taskrunner into TUScheduler and block its
> >> destruction. We can just re-use the existing TaskRunner inside
> >> parsingcallbacks, in which we run stdlib indexing tasks.
>
> and in the follow up;
>
> >> I think we can just change the signature for PreambleParsedCallback to
> >> pass along refcounted objects. forgot to mention in the first comment, but
> >> we should also change the CanonicalIncludes to be a shared_ptr so that it
> >> can outlive the PreambleData. We should invoke the callback inside
> >> buildPreamble after a successful build. Afterwards we should also change
> >> the signature for onPreambleAST to take AST, PP and CanonicalIncludes as
> >> ref-counted objects again and PreambleThread::build should just forward
> >> objects received from PreambleParsedCallback. Afterwards inside the
> >> UpdateIndexCallbacks::onPreambleAST we can just invoke indexing on Tasks
> >> if it's present or synchronously in the absence of it.
>
> I was pointing towards running this inside the `Tasks` in
> `UpdateIndexCallbacks`.
>
> ---
>
> There's definitely some upsides to running that indexing on the preamble
> thread as well (which is what we do today) but I think the extra sequencing
> requirements (make sure to first notify the ASTPeer and then issue preamble
> callbacks) we put into TUScheduler (which is already quite complex) is
> probably not worth it.
Thanks again for the review. Updated the diff such that indexing is now in
IndexTasks. AFIK, there will be a single thread that will now manage indexing
of preamble for all the opened TUs. Please let me know if you see any issues
there.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D148088/new/
https://reviews.llvm.org/D148088
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
