As discussed offline this definitely LGTM, let me summarize the reasoning here.
The original review was in https://reviews.llvm.org/D41365. It's unclear why they went with before BeginSourceFile. In theory having the callback issued before BeginSourceFile allows setting various options that'll effect parsing, but today there's only one user (at least in the upstream) of this callback, clangd, which uses the callack to stash some state from CompilerIntance rather than mutating it.
If we ever have users that'll need to mutate the CompilerIntance before BeginSourceFile, I think we should have a particular callback for that use case then.
Will still wait for feedback from @sammccall as he might have some historical context.
Yeah this change definitely looks safe:
- we've moving it across BeginSourceFile only, which doesn't seem to do anything for the usual parse that we'd use when building preambles.
- BeforeExecute isn't widely used
Also it seems saner: Before/AfterExecute are now consistently nested inside Begin/EndSourceFile.
I don't have particular insight into the original motivation, but I guess it was just driven by what was needed: why go for the "more powerful" thing if we only needed to inspect the CompilerInstance?