On Oct 4, 2011, at 1:07 PM, Johan Engelen wrote:
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...)
Actually the @brief tags makes it harder for newcomers, and also different problems for the developers needing to maintain and add such comments. Aside from helping the newbies on IRC, Twitter, etc. get going, I've been doing this sort of thing at my day job for years now. Most of that includes coordinating with personnel not only in different timezones, but also spread across many continents. So even though my experience is not solely from open source, the working environment has been very similar.
The reason I'm spending time on the 'waste' is that I am trying to get our code documented better, and restructuring from the legacy C approach to the C++ that has replaced it is a big part.
In order to get a good architectural overview, Doxygen output is a key tool. And if we don't have good Doxygen output, it's hard to see what's going on. It's also hard for newcomers to use it.
So the effort is part of the overall work to have diagrams, charts, maps of where to find what, etc., that will be exactly what newcomers need. And I should also be able to get some templates, guidelines, etc. to help the rest of us gain maximum use with minimal effort.
Oh, and for a more quantitative evaluation, I ran a spot-check shortly before finishing the cleanup of src/forward.h and came up with the following:
65 total spot-checked files
22 removed forward.h and needed 1 forward declaration added 19 removed forward.h and needed 0 forward declarations added 8 removed forward.h and needed 3 forward declarations added 7 had no forward.h but needed 1 forward declaration added 6 removed forward.h and needed 2 forward declarations added 1 had no forward.h but needed 1 forward declaration added 1 removed forward.h and needed 4 forward declarations added 1 removed forward.h and needed 7 forward declarations added
So we have about two-thirds of the files where there was no real need, and almost half of that was due to forward.h being included when there was no need at all. Then only one file of the total would be considered potentially problematic and worth looking into.
(For running the check, I had each .h file individually compiled into a .cpp where including that .h was the only thing done.)
Please do not remove the awesome "#include <2geom/forward.h>". It is very concise, and much better than handwriting the whole namespacing etc...
That's a slightly different problem.
For that, you are looking at an 'I want to use some library and not worry' situation. For that purpose, I'd say that <2geom/forward.h> is a bit misleading. A better approach would be to narrow down to some specific types. Or if you really want the convenience of using the library without needing to worry, look for a full-name kind of include-the-whole-world file. Ah, there we go with <2geom/2geom.h>
*However* often actually being able to *use* a library will not be safely served via forward includes. To save space in one's header, a few sub-component includes might be more helpful. However, even 2geom's current forward.h appears to be broken.
If I look at effect.h (a true case of a somewhat 'interesting' header), there are a few causes for concern. As expected, it does have a few stale declarations that should be removed. However, another one is that a lot of use of forward declared types abounds, including in std::vector. In the general case this is not correct code, and may not work (depending on the compiler, etc.)
http://www.parashift.com/c++-faq-lite/misc-technical-issues.html#faq-39.14
However, in this specific it turns out that other #includes already pulled in the full files. I did a quick spot check and found that one can remove the #include of 2geom/forward.h completely and other #includes already have things covered. Of course, I don't encourage this, but it is a clear sign that we might have a lot of code pulling in more than we expect, and that some forward declarations are only working due to side-effects of other #includes.
At some point we should look into such aspects in more detail.