jkorous added inline comments.
================ Comment at: clang/lib/Index/IndexingAction.cpp:77 + IndexCtx->getDataConsumer().setPreprocessor(PP); + PP->addPPCallbacks(std::make_unique<IndexPPCallbacks>(IndexCtx)); + } ---------------- gribozavr wrote: > 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. We could also move the preprocessor setup to constructor and check/add asserts that `ASTContext` has been provided to `IndexingContext` and `IndexDataConsumer` implementations. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66877/new/ https://reviews.llvm.org/D66877 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
