[PATCH] D66877: Moved the IndexDataConsumer::finish call into the IndexASTConsumer from IndexAction

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr marked 2 inline comments as done.
gribozavr added inline comments.


================
Comment at: clang/lib/Index/IndexingAction.cpp:77
+    IndexCtx->getDataConsumer().setPreprocessor(PP);
+    PP->addPPCallbacks(std::make_unique<IndexPPCallbacks>(IndexCtx));
+  }
----------------
ilya-biryukov wrote:
> The fact that we call `addPPCallbacks` **after** creating `ASTContext` is a bit concerning.
> 
> This wouldn't probably cause any problems in practice, but generally changing preprocessor by the time `ASTContext` is already created seems dangerous (what if something there ever starts depending on the state of the preprocessor?)
> 
> I think this concern is easily elevated if we change Preprocessor and call addPPCallbacks on `CreateASTConsumer(CompilerInvocation& CI)`. That would mean we have a separate function that sets up the preprocessor and creates `IndexASTConsumer` and it should be exposed to the clients.
> 
> Have you considered this approach? Would it work?
It does feel a bit weird, but we shouldn't have started parsing before calling `Initialize` on the `ASTConsumer`. Therefore I agree with you that it won't cause problems in practice.

Calling `addPPCallbacks` in `FrontendAction` is against the goal of this patch set. The goal is to encapsulate as much as possible in the `IndexASTConsumer`, because they compose well, unlike `FrontendAction`s. Therefore, requiring customization in `FrontendAction` is not possible.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66877/new/

https://reviews.llvm.org/D66877