Thread: Removing useless #include's.

Hello.

While looking some patch, just from curiosity, I checked for
redundant #include's in the source tree (except
contrib). "redundant" here means that a file is included in
another include file nearby.

I found 641 includes that is just removable with no side effect
with two exceptions.

- src/common/saslprep.c

  A comment that suggests linking wchar.c was placed just above
  '#include "mb/pg_wchar.h" but it is now just above "#include
  "common/unicode_norm.h" but the comment seems to be used as is.

 
- backend/storage/lmgr/spin.c

  spin.c and spin.h don't aggree on the necessity of pg_sema.h
  when HAVE_SPINLOCKS is defined. I post a mail about this issue
  separately.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes:
> While looking some patch, just from curiosity, I checked for
> redundant #include's in the source tree (except
> contrib). "redundant" here means that a file is included in
> another include file nearby.

> I found 641 includes that is just removable with no side effect
> with two exceptions.

I tend to be a bit suspicious of this sort of thing, especially for
old files that have been through previous rounds of "unnecessary
include" removal.  It's usually a good idea to ask *why* is a header
no longer needed?  The answer, usually, is that somebody added the
same #include to some other header, and it's not uncommon for that
to have been a bad idea.  It's usually best to minimize cross-header
inclusions, IMV, and it's always necessary to exercise judgment
when adding one.

We've also had more than a few problems with automatic scripts deciding
that an #include could be removed because they weren't testing with the
combination of build options that made it necessary.

See for instance commits 6416a82a6 through 1609797c2 for some history
of poorly managed #include removal.

            regards, tom lane

Hello.

At Thu, 15 Feb 2018 11:12:05 -0500, Tom Lane <tgl@sss.pgh.pa.us> wrote in <6748.1518711125@sss.pgh.pa.us>
> Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes:
> > While looking some patch, just from curiosity, I checked for
> > redundant #include's in the source tree (except
> > contrib). "redundant" here means that a file is included in
> > another include file nearby.
> 
> > I found 641 includes that is just removable with no side effect
> > with two exceptions.
> 
> I tend to be a bit suspicious of this sort of thing, especially for
> old files that have been through previous rounds of "unnecessary
> include" removal.  It's usually a good idea to ask *why* is a header
> no longer needed?  The answer, usually, is that somebody added the
> same #include to some other header, and it's not uncommon for that
> to have been a bad idea.  It's usually best to minimize cross-header
> inclusions, IMV, and it's always necessary to exercise judgment
> when adding one.

As another point of view, placing an #include just because the
source file uses the definition in the file is also
reasonable. Header files are kept not to have a problem when
included multiple times. I don't surprise if someone says that
this is rather harmful. And I'm glas to read the clear reason.

> We've also had more than a few problems with automatic scripts deciding
> that an #include could be removed because they weren't testing with the
> combination of build options that made it necessary.
> 
> See for instance commits 6416a82a6 through 1609797c2 for some history
> of poorly managed #include removal.

I'm surprised to find no circular/mutual dependency even nor
multilevel inclusion among header files. I think I understand the
reason.

In this patch, I didn't touch the header files since I felt that
somewhat dangerous. But anyway I understand doing all of this at
a time can be dangerous.

Thank you for the suggestion, Tom.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes:
> At Thu, 15 Feb 2018 11:12:05 -0500, Tom Lane <tgl@sss.pgh.pa.us> wrote in <6748.1518711125@sss.pgh.pa.us>
>> Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes:
>>> I found 641 includes that is just removable with no side effect
>>> with two exceptions.

>> I tend to be a bit suspicious of this sort of thing, especially for
>> old files that have been through previous rounds of "unnecessary
>> include" removal.

> As another point of view, placing an #include just because the
> source file uses the definition in the file is also
> reasonable. Header files are kept not to have a problem when
> included multiple times. I don't surprise if someone says that
> this is rather harmful. And I'm glas to read the clear reason.

Note that I'm *not* saying there's nothing to be done here.  Scanning
through your patch quickly, the #include "postgres.h" in knapsack.h
is clearly contrary to project policy, and there should surely be
no need for any backend .c file to #include elog.h or palloc.h
because postgres.h pulls in both of those.  But I'd like to see a bit
more analysis of most of the rest of these changes.  The bad experience
we had with the last round of #include-removal was really because some
poor decisions had been taken before that about which headers should
include which other headers.  I think it'd be a good idea to work backward
and see whether any of these proposed changes are pointing to header
inclusions that maybe weren't well thought out.

            regards, tom lane