Re: Feature branch launched: deheader

On Mon, 6 Dec 2010 07:48:48 +1300, Robert Collins
<robertc_at_robertcollins.net> wrote:
> On Mon, Dec 6, 2010 at 3:28 AM, Kinkie <gkinkie_at_gmail.com> wrote:
>> Hi all,
>>  Eric Raymond recently released a tool named deheader
>> (http://www.catb.org/esr/deheader/) which goes through c/c++ project
>> looking for unneeded includes.
>> It does so by trying to compile each source file after removing one
>> #include statement at a time, and seeing if it builds.
>
> Thats a fairly flawed approach.
>
> Two reasons:
> - some headers when present affect correctness, not compilation.
> - on some platforms headers are mandatory, on others optional.
>
> So, if you do this, be sure to take the minimal approach after
> building on *all* platforms, and then still be conservative.
>
> -Rob

Aye, to avoid those two problems, DO NOT make any changes in the compat/
folder indicated by deheader.
Where an OS has mandatory headers the include likely should be moved to
the OS area of compat/, common sense applies of course when the include is
component specific. Comments in the code next to those includes is required
regardless of where they are.

If a header is included through the general/shared area of compat the src/
copy may be removable. Otherwise duplicates are required. Note that compat
may/should #undef the HAVE_* for files included there to save build time.

On the whole I would take a slightly different approach. Using deheader in
a multiple-pass approach:

 pass 0: manually check the headers adding class pre-defines wherever
possible but missing. This will fix a lot of false-negatives the deheader
approach WILL hit in the squid sources.

 pass 1: to process the testHeaders pseudo-apps and remove the
sub-includes which are not actually needing to be pulled in via the .h.
This will highlight many bugs in the real .c/.cc which fail to include
their dependency .h directly.

 pass 2: running it no the whole project to see how many of the .c/.cc
includes are redundant. Using this as a rough guide to problems, noting
that there are likely a great many false-positives for the reasons Rob
mentions in addition to component dependency permutations.

As the least it will be a way to improve the code documentation about why
certain headers are included in strange places.

I'm wondering if we should collate the reports from testing tools like
this for manual inspection somewhere. I have cppcheck and valgrind outputs
as well that anyone could take up as a minor task. I suspect Alex has more
of those or from other tools as well.

Amos
Received on Sun Dec 05 2010 - 23:52:03 MST