[systemd-devel] Removing unnecessary includes

[Thomas H.P. Andersen]

On Tue, Feb 10, 2015 at 10:13 PM, Lennart Poettering
<lennart at poettering.net> wrote:
> On Mon, 09.02.15 10:19, Thomas H.P. Andersen (phomes at gmail.com) wrote:
>
>> 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.
>
> Hmm, I find this unnecessary noise I must say...

Well, for someone who is still learning it can be useful :) Getting
that list of symbols actually used from each header adds some context
and makes the includes feel less magic. Anyway, something to use on
the fly but not to commit of course. There is an option to turn it off
so that is not a problem.

>> #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
>
> This one we get though "-include config.h" on the gcc command line.

We can configure it to understand that.

>> #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
>
> Nah, please no absolute includes...
>
> Any chance this can turned off?

I did not find any option for that but it is something to look into.

>> 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
>
> Note that I actually try to roughly maintain an order when including
> things: For the headers of other projects i try to put system headers
> first, 3rd party headers second. For our own stuff I try to put
> external API stuff first (i.e. sd-*.h), followed by internal API-like
> stuff form src/shared, and finally the other headers that are prviate
> to the specific module the code is in.
>
> I'd really like to avoid a scheme where this is reordered
> randomly...

Understood. The include-what-you-use tool consists of two separate
parts. The first is a program to scan the code and create a report of
what to add and remove. The second part is a python script that reads
the report and patches the source files. Perhaps the latter be
extended to do what we need.

> The order kinda is relevant, since more "local", "specific"
> definitions should never influence more "public", "generic"
> interfaces, if you follow what I mean? A lot of header files use
> #ifndef, and only conditionally define things then. They should not
> get confused by our own definitions....

Yep. Makes sense.

Here is a status on what I have done so far.

include-what-you-want does the following:
1) sorts the includes
2) adds missing headers for any symbols used
3) adds forward declarations
4) removes any unused headers (after step 2+3)
5) changes some headers. (only saw <sys/poll.h> to <poll.h> for now)

The diff we get out of that is too big a mess to locate what we want:
the currently unused headers. To break it up I first did the sorting
in separate step. (I have a patch to commit after 219 for some minor
issues that came up from that). I then started to look at all the
removals and one by one see if they make sense today, or was due to
step 2/3/5, or was something we want to keep like missing.h. It is
slow manual work but I will get there.

It would be helpful to know if we might want 2, 3, and 5 done?