[PATCH] D148088: [RFC][clangd] Move preamble index out of document open critical path

[Kadir Cetinkaya via Phabricator via cfe-commits]

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