kadircet added a comment.
i think something went wrong with the diff, you don't seem to update
PreambleCallbacks to trigger indexing on a different thread at all (and also
there are certain lifetime issues). is this the final version of the patch or
did I bump into a WIP version unknowingly ?
================
Comment at: clang-tools-extra/clangd/Preamble.cpp:106-113
+ CI.setSema(nullptr);
+ CI.setASTConsumer(nullptr);
+ if (CI.getASTReader()) {
+ CI.getASTReader()->setDeserializationListener(nullptr);
+ /* This just sets consumer to null when DeserializationListener is null
*/
+ CI.getASTReader()->StartTranslationUnit(nullptr);
}
----------------
why are we triggering destructors for all of these objects eagerly? if this is
deliberate to "fix" some issue, could you mention that in 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);
----------------
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)
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:1063-1071
+ if (LatestBuild) {
+ assert(CapturedCtx);
+ CanonicalIncludes CanonIncludes = LatestBuild->CanonIncludes;
+ CompilerInvocation &CI = CapturedCtx->getCompilerInvocation();
+ ASTContext &Ctx = CapturedCtx->getASTContext();
+ Preprocessor &PP = CapturedCtx->getPreprocessor();
+ Callbacks.onPreambleAST(FileName, Inputs.Version, CI, Ctx, PP,
----------------
you can just execute this block around the call to `reportPreambleBuild` right?
is there any particular reason to make this part of scope cleanup ? especially
call it after the call to `updatePreamble` ?
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:1066-1068
+ CompilerInvocation &CI = CapturedCtx->getCompilerInvocation();
+ ASTContext &Ctx = CapturedCtx->getASTContext();
+ Preprocessor &PP = CapturedCtx->getPreprocessor();
----------------
these are all references to `CapturedCtx` which will be destroyed after the
call. i guess we should be passing shared_ptrs/IntrusiveRefCntPtrs instead,
right?
also I don't think it's enough to just pass these 3. we need to make sure rest
of the fields in CapturedCtx is also kept alive (e.g. Preprocessor has raw
pointers to Target/AuxTarget, which would be dangling after CapturedCtx goes
out of scope).
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
