When doing refactorings in header files, such as ExprEngine.h or similar headers, then basically all the CSA TUs need to be recompiled because we depend on those headers transitively. I’d say, it results in a pretty bad experience to rebuild like ~150 TUs. Especially, because IMO most of the checkers should not depend on such headers. This discourages doing gradual cleanups.
My primary objective would be not to improve build times, but rather decouple thinks, so that we don’t need to recompile everything if some header changes.
I created a downstream patch stack of ~8 commits, erasing some header dependencies by replacing includes with forward-declarations. This didn’t improve the situation by large, and some of the commits basically touch all the files, thus I’m not sure of the return-of-investment for landing them.
Given these, are you interested in reviewing a stream of NFC commits, basically moving includes to cpp files out of headers - and putting a bunch of forward declarations there instead.
This sounds valuable, I totally support this and I don’t see any downsides. We already have quite a bit of such forward declarations. If we suddenly really need access to more stuff, we can always bring back includes as needed.
To give an example, lets say we want to get rid of the BugReporter as the addTransition only really need a forward declaration on the public API.
However, all checkers want at some point report bugs that means that now all checkers need to include the BugReporter header separately anyway.
This shifts more includes to the corresponding CPP files, from headers.
Actually, its already the case for the BugType header.
Rarely any API needs that header, but basically all checkers need it.
So, it might make sense to actually merge some of the headers or have an umbrella header for the usual checker stuff.
Other than the include burden in headers, some of the compile time errors for missing an include for the forward decl might not be obvious for newcomers. Also, clangd doesn’t offer fixit for including some of such dependencies. (It works for the most part though)
So in fact, I feel it has its downsides as well. We might wanna apply it selectively, but I didn’t have that in mind when did the refactoring.
We already have that for path-sensitive checkers; it’s CheckerContext.h. Checker context is exactly what nobody needs in the engine but every checker needs.
On the other hand, probably the only thing that path-insensitive checkers need is bug reporting stuff.
Makes sense. So the checker context should be the glorified header file for forward declarations for the non-necessary stuff, such as AnalysisManager etc. And include the really must have headers such as BugType and BugReporter.
Then we should ensure that those transitive headers are as slim as possible.
that means that now all checkers need to include the BugReporter header separately anyway.