[systemd-devel] Removing unnecessary includes

[Thomas H.P. Andersen]

On Sat, Feb 7, 2015 at 3:38 PM, Thomas H.P. Andersen <phomes at gmail.com> wrote:
> On Sat, Feb 7, 2015 at 2:37 PM, Ronny Chevalier
> <chevalier.ronny at gmail.com> wrote:
>> 2015-02-07 14:05 GMT+01:00 Daniele Nicolodi <daniele at grinta.net>:
>>> On 07/02/15 10:29, Thomas H.P. Andersen wrote:
>>>> I am looking at ways to automatically trim the unnecessary includes.
>>>> One way to do it is a script[1] which simply tests if the compile
>>>> still works after removing each include one at a time. It does this in
>>>> reverse order for all includes in the .c files. Using -Werror we catch
>>>> any new warnings too.
>>>
>>> Hello Thomas,
>>>
>>> this approach is not correct: in this way each source file would not be
>>> required to include the headers included by other files included before.
>>> For example, if header file "a.h" includes "shared.h" and implementation
>>> file requires the definitions of "a.h" and "shared.h", only the first
>>> dependency would be detected by this method.
>>>
>>> However, it is good practice to include all the required header files,
>>> whether those are already included by others or not.
>>>
>>
>> Hi,
>>
>> I agree with Daniele. If you want to include the proper headers in
>> each file maybe you can use include-what-you-use [0], but this is a
>> rather recent project with lots of issues that will force you to do a
>> lots of manual review.
>>
>> [0] https://code.google.com/p/include-what-you-use/
>
> Looks useful. Will take a look.

include-what-you-use is actually pretty nice. It is also a little bit
crazy. It wants to include everything directly and we would add a lot
includes for errno.h, string.h, stdlib.h, stdbool.h, stddef.h, etc etc
everywhere. It also wants to use forward declares when possible. The
coding style does not say anything specific about this. Any
preferences? I will use the tool to trim the unnecessary includes
first and leave its other advise for later. It will be interesting to
see how each step affects the build time.

A feature is that I find interesting is that it can comment each
include with the reason that it was included. Not something to commit,
but useful to get an overview of why each include is there.

Include-what-you-use is not packaged yet in fedora and you need to
edit the cmake files to make it build there. Dave Johansen is working
on the packaging though and it looks like it could go in any day now.
https://bugzilla.redhat.com/show_bug.cgi?id=1091659

To run it you need something like:
./autogen.sh c && make -k CC=include-what-you-use

Here is an example of the advise it gives for a single file:

src/analyze/analyze.c should add these lines:
#include <errno.h>                      // for EIO, ENOMEM, E2BIG, EINVAL, etc
#include <stdbool.h>                    // for false, true, bool
#include <stddef.h>                     // for offsetof, size_t
#include <stdint.h>                     // for uint64_t
#include <string.h>                     // for strdup, strlen
#include "config.h"                     // for PACKAGE_STRING, VERSION
#include "src/shared/macro.h"           // for assert, assert_cc, etc
#include "src/shared/path-lookup.h"
#include "src/shared/time-util.h"       // for format_timespan, usec_t, etc
#include "src/systemd/sd-bus-protocol.h"  // for ::SD_BUS_TYPE_ARRAY

src/analyze/analyze.c should remove these lines:
- #include <sys/utsname.h>  // lines 27-27
- #include "fileio.h"  // lines 38-38
- #include "install.h"  // lines 33-33

The full include-list for src/analyze/analyze.c:
#include <errno.h>                      // for EIO, ENOMEM, E2BIG, EINVAL, etc
#include <fnmatch.h>                    // for fnmatch
#include <getopt.h>                     // for optind, no_argument, optarg, etc
#include <locale.h>                     // for NULL, setlocale, LC_ALL, etc
#include <stdbool.h>                    // for false, true, bool
#include <stddef.h>                     // for offsetof, size_t
#include <stdint.h>                     // for uint64_t
#include <stdio.h>                      // for printf, puts, fputs, stdout
#include <stdlib.h>                     // for free, qsort, EXIT_FAILURE, etc
#include <string.h>                     // for strdup, strlen
#include "analyze-verify.h"             // for verify_units
#include "build.h"                      // for SYSTEMD_FEATURES
#include "bus-error.h"                  // for bus_error_message
#include "bus-util.h"                   // for UnitInfo, etc
#include "config.h"                     // for PACKAGE_STRING, VERSION
#include "hashmap.h"                    // for hashmap_get, Hashmap, etc
#include "log.h"                        // for log_error, log_oom, etc
#include "pager.h"                      // for pager_close, pager_open
#include "sd-bus.h"                     // for sd_bus, SD_BUS_ERROR_NULL, etc
#include "special.h"                    // for SPECIAL_DEFAULT_TARGET
#include "src/shared/macro.h"           // for assert, assert_cc, etc
#include "src/shared/path-lookup.h"
#include "src/shared/time-util.h"       // for format_timespan, usec_t, etc
#include "src/systemd/sd-bus-protocol.h"  // for ::SD_BUS_TYPE_ARRAY
#include "strv.h"                       // for strv_isempty, STRV_FOREACH, etc
#include "strxcpyx.h"                   // for strpcpyf
#include "unit-name.h"                  // for unit_dbus_path_from_name
#include "util.h"                       // for isempty, streq, etc