ilya-biryukov accepted this revision.
ilya-biryukov marked an inline comment as done.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.
LGTM
================
Comment at: clang/lib/Index/IndexingAction.cpp:77
+ IndexCtx->getDataConsumer().setPreprocessor(PP);
+ PP->addPPCallbacks(std::make_unique<IndexPPCallbacks>(IndexCtx));
+ }
----------------
jkorous wrote:
> 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.
LG, let's keep it like this and hope this won't break in the future too.
And if it will, we would probably be able to fix this by setting up
`PPCallbacks` at the time when we create the `ASTConsumer`, i.e.
```
std::unique_ptr<ASTConsumer> createIndexASTConsumer(CompilerInvocation &CI) {
CI.getPreprocessor().addPPCallback();
return llvm::make_unique<IndexASTConsumer>(...);
}
```
That would reduce the gap between `FrontendAction::BeginSourceFile` and
`addPPCallbacks` to just a few instructions, and the chances of breaking stuff
would be even less.
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
