On 4-10-2011 21:48, Jon Cruz wrote:
On Oct 4, 2011, at 9:27 AM, Krzysztof Kosiński wrote:
Hello I noticed that Jon Cruz is removing forward headers and replacing them with hand-written forward declarations. This is completely backwards! Jon why are you doing this?
Quite simple: I'm not undoing the work, I'm finishing it.
Forward declarations are good, but forward declaration files are not really.
One main rule we have in play here is "Don't include headers you don't need. Especially in headers." And often also "make headers independent".
There are two main benefits:
reduces compile time by avoiding pulling in additional headers.
minimizes rebuild times when one header is changed.
So we remove #including of detailed extra headers. That is good. However, if we replace that with "let's include this *different* file instead", then we lose much of the benefit. Though that one forward.h might compile a little faster, it still has to be checked and consulted. Also it introduces an artificial dependency that when touched will cascade out and cause more files to be compiled than should. (The main example we have in our source code of such cascading is verbs.h - touch that and the whole world rebuilds)
The full solution is to just forward-declare the single items *used* within a given header. No extra #includes need to be added. It also makes those .h files independent of each other.
Take a look at the changes I was making. Among other things you should see that though there might be dozens of declarations in a forward.h, any given .h file only needs perhaps one or two of those.
Then over time the forward.h files tend to accumulate cruft. If the have outdated or sometimes wrong declarations, or unused ones, they just add to the potential for slowdown or error. On the other hand if forward declarations are in a file that needed them, one can very trivially search that one .h file to see if they are used. If not, they can be removed with no problem. Much easier to keep in sync and free of cruft. And safer to do so also (especially when one considers conditional or platform specific code). And safer code is good code.
Another potential problem is the naming. A work-around is to prefix a forward.h with some other name, but we still end up with at least a few potential collisions. On the other hand, if each .h file is independent with it's own few declarations, then there does not even exist the possibility of needing a different file with the same name.
So picture a specific method being added that takes a reference to some type. One approach is for the coder to add a forward declaration for that type in the .h file that he is also adding the method to. One file touched, and only consumers of that specific file get rebuilt.
The alternative is to place the type in a forward.h. So in that case the coder has to edit the .h to add a new method, also add a new #include line to that same .h and expand it's dependencies, and finally has to also edit the pertinent forward.h file to add a declaration for that type. At least two .h files get changed, but since one is a .h file that is pulled into many other .h files *and* .cpp files (though .cpp files should never use them, they often end up accidentally having them). So now many dozens of .cpp files get rebuilt in response to what should have been an isolated case.
To be fair, that scenario is a bit less frequent, but we have a choice of following a practice that ensures it *never* gets to happen vs. one that let's it *sometimes* happen, we should always go for the code enforcing the safer of never allowing the problem.
I guess there will always be discussion on what is "best" with these forward declarations. Personally, I hate it when there are all these meaningless forward declaration lines at the start of a file that I have to scroll past. I find that 'cleaning up' code one way or the other is simply a waste of time that would have been better spent otherwise. (like the "@brief" removal that I do not understand; if anything, it makes doxygen stuff more clear to new-comers...)
Please do not remove the awesome "#include <2geom/forward.h>". It is very concise, and much better than handwriting the whole namespacing etc...
Ciao, Johan