Unused header file inclusion

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
30 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Unused header file inclusion

vignesh C
Hi,

I noticed that there are many header files being
included which need not be included.
I have tried this in a few files and found the
compilation and regression to be working.
I have attached the patch for the files that
I tried.
I tried this in CentOS, I did not find the header
files to be platform specific.
Should we pursue this further and cleanup in
all the files?

Regards,
Vignesh

unused_header_file_exclusion.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Unused header file inclusion

Michael Paquier-2
On Wed, Jul 31, 2019 at 11:19:08AM +0530, vignesh C wrote:
> I noticed that there are many header files being included which need
> not be included.  I have tried this in a few files and found the
> compilation and regression to be working.  I have attached the patch
> for the files that  I tried.  I tried this in CentOS, I did not find
> the header files to be platform specific.
> Should we pursue this further and cleanup in all the files?

Do you use a particular method here or just manual deduction after
looking at each file individually?  If this can be cleaned up a bit, I
think that's welcome.  The removal of headers is easily forgotten when
moving code from one file to another...
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Unused header file inclusion

vignesh C
On Wed, Jul 31, 2019 at 11:26 AM Michael Paquier <[hidden email]> wrote:

>
> On Wed, Jul 31, 2019 at 11:19:08AM +0530, vignesh C wrote:
> > I noticed that there are many header files being included which need
> > not be included.  I have tried this in a few files and found the
> > compilation and regression to be working.  I have attached the patch
> > for the files that  I tried.  I tried this in CentOS, I did not find
> > the header files to be platform specific.
> > Should we pursue this further and cleanup in all the files?
>
> Do you use a particular method here or just manual deduction after
> looking at each file individually?  If this can be cleaned up a bit, I
> think that's welcome.  The removal of headers is easily forgotten when
> moving code from one file to another...
>
Thanks Michael.
I'm writing some perl scripts to identify this.
The script will scan through all the files, make changes,
and verify.
Finally it will give the changed files.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: Unused header file inclusion

akapila
On Wed, Jul 31, 2019 at 11:31 AM vignesh C <[hidden email]> wrote:

>
> On Wed, Jul 31, 2019 at 11:26 AM Michael Paquier <[hidden email]> wrote:
> >
> > On Wed, Jul 31, 2019 at 11:19:08AM +0530, vignesh C wrote:
> > > I noticed that there are many header files being included which need
> > > not be included.  I have tried this in a few files and found the
> > > compilation and regression to be working.  I have attached the patch
> > > for the files that  I tried.  I tried this in CentOS, I did not find
> > > the header files to be platform specific.
> > > Should we pursue this further and cleanup in all the files?
> >
> > Do you use a particular method here or just manual deduction after
> > looking at each file individually?  If this can be cleaned up a bit, I
> > think that's welcome.  The removal of headers is easily forgotten when
> > moving code from one file to another...
> >
> Thanks Michael.
> I'm writing some perl scripts to identify this.
> The script will scan through all the files, make changes,
> and verify.

If we can come up with some such tool, we might be able to integrate
it with Thomas's patch tester [1] wherein it can apply the patch,
verify if there are unnecessary includes in the patch and report the
same.

[1] - http://commitfest.cputube.org/

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: Unused header file inclusion

vignesh C
On Wed, Jul 31, 2019 at 11:55 AM Amit Kapila <[hidden email]> wrote:

>
> On Wed, Jul 31, 2019 at 11:31 AM vignesh C <[hidden email]> wrote:
> >
> > On Wed, Jul 31, 2019 at 11:26 AM Michael Paquier <[hidden email]> wrote:
> > >
> > > On Wed, Jul 31, 2019 at 11:19:08AM +0530, vignesh C wrote:
> > > > I noticed that there are many header files being included which need
> > > > not be included.  I have tried this in a few files and found the
> > > > compilation and regression to be working.  I have attached the patch
> > > > for the files that  I tried.  I tried this in CentOS, I did not find
> > > > the header files to be platform specific.
> > > > Should we pursue this further and cleanup in all the files?
> > >
> > > Do you use a particular method here or just manual deduction after
> > > looking at each file individually?  If this can be cleaned up a bit, I
> > > think that's welcome.  The removal of headers is easily forgotten when
> > > moving code from one file to another...
> > >
> > Thanks Michael.
> > I'm writing some perl scripts to identify this.
> > The script will scan through all the files, make changes,
> > and verify.
>
> If we can come up with some such tool, we might be able to integrate
> it with Thomas's patch tester [1] wherein it can apply the patch,
> verify if there are unnecessary includes in the patch and report the
> same.
>
> [1] - http://commitfest.cputube.org/
>
Thanks Amit.
I will post the tool along with the patch once I complete this activity. We
can enhance the tool further based on feedback and take it forward.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: Unused header file inclusion

Michael Paquier-2
In reply to this post by akapila
On Wed, Jul 31, 2019 at 11:55:37AM +0530, Amit Kapila wrote:
> If we can come up with some such tool, we might be able to integrate
> it with Thomas's patch tester [1] wherein it can apply the patch,
> verify if there are unnecessary includes in the patch and report the
> same.
>
> [1] - http://commitfest.cputube.org/

Or even get something into src/tools/?  If the produced result is
clean enough, that could be interesting.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Unused header file inclusion

Andres Freund
In reply to this post by vignesh C
Hi,

On 2019-07-31 11:19:08 +0530, vignesh C wrote:

> I noticed that there are many header files being
> included which need not be included.
> I have tried this in a few files and found the
> compilation and regression to be working.
> I have attached the patch for the files that
> I tried.
> I tried this in CentOS, I did not find the header
> files to be platform specific.
> Should we pursue this further and cleanup in
> all the files?

These type of changes imo have a good chance of making things more
fragile. A lot of the includes in header files are purely due to needing
one or two definitions (which often could even be avoided by forward
declarations). If you remove all the includes directly from the c files
that are also included from some .h file, you increase the reliance on
the indirect includes - making it harder to clean up.

If anything, we should *increase* the number of includes, so we don't
rely on indirect includes.  But that's also not necessarily the right
choice, because it adds unnecessary dependencies.


> --- a/src/backend/utils/mmgr/freepage.c
> +++ b/src/backend/utils/mmgr/freepage.c
> @@ -56,7 +56,6 @@
>  #include "miscadmin.h"
>  
>  #include "utils/freepage.h"
> -#include "utils/relptr.h"

I don't think it's a good idea to remove this header, for example. A
*lot* of code in freepage.c relies on it. The fact that freepage.h also
includes it here is just due to needing other parts of it


>  /* Magic numbers to identify various page types */
> diff --git a/src/backend/utils/mmgr/portalmem.c b/src/backend/utils/mmgr/portalmem.c
> index 334e35b..67268fd 100644
> --- a/src/backend/utils/mmgr/portalmem.c
> +++ b/src/backend/utils/mmgr/portalmem.c
> @@ -26,7 +26,6 @@
>  #include "utils/builtins.h"
>  #include "utils/memutils.h"
>  #include "utils/snapmgr.h"
> -#include "utils/timestamp.h"

Similarly, this file uses timestamp.h functions directly. The fact that
timestamp.h already is included is just due to implementation details of
xact.h that this file shouldn't depend on.

Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: Unused header file inclusion

Tom Lane-2
In reply to this post by Michael Paquier-2
Michael Paquier <[hidden email]> writes:
> On Wed, Jul 31, 2019 at 11:55:37AM +0530, Amit Kapila wrote:
>> If we can come up with some such tool, we might be able to integrate
>> it with Thomas's patch tester [1] wherein it can apply the patch,
>> verify if there are unnecessary includes in the patch and report the
>> same.

> Or even get something into src/tools/?  If the produced result is
> clean enough, that could be interesting.

I take it nobody has actually bothered to *look* in src/tools.

src/tools/pginclude/README

Note that our experience with this sort of thing has not been very good.
See particularly 1609797c2.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: Unused header file inclusion

Alvaro Herrera-9
In reply to this post by vignesh C
On 2019-Jul-31, vignesh C wrote:

> I noticed that there are many header files being
> included which need not be included.

Yeah, we have tooling for this already in src/tools/pginclude.  It's
been used before, and it has wreaked considerable havoc; see "git log
--grep pgrminclude".

I think doing this sort of cleanup is useful to a point -- as Andres
mentions, some includes are somewhat more "important" than others, so
judgement is needed in each case.

I think removing unnecessary include lines from header files is much
more useful than from .c files.  However, nowadays even I am not very
convinced that that is a very fruitful use of time, since many/most
developers use ccache which will reduce the compile times anyway in many
cases; and development machines are typically much faster than ten years
ago.

Also, I think addition of new include lines to existing .c files should
be a point worth specific attention in patch review, to avoid breaking
reasonable modularity boundaries unnecessarily.

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: Unused header file inclusion

Andres Freund
Hi,

On 2019-07-31 11:23:22 -0400, Alvaro Herrera wrote:
> I think removing unnecessary include lines from header files is much
> more useful than from .c files.  However, nowadays even I am not very
> convinced that that is a very fruitful use of time, since many/most
> developers use ccache which will reduce the compile times anyway in many
> cases; and development machines are typically much faster than ten years
> ago.

IDK, I find the compilation times annoying. And it's gotten quite
noticably worse with all the speculative execution mitigations. Although
to some degree that's not really the fault of individual compilations,
but our buildsystem being pretty slow.

I think there's also just modularity reasons for removing includes from
headers. We've some pretty oddly interlinked systems, often without good
reason (*). Cleaning those up imo is well worth the time - but hardly
can be automated.

If one really wanted to automate removal of header files, it'd need to
be a lot smarter than just checking whether a file fails to compile if
one header is removed. In the general case we'd have to test if the .c
file itself uses any of the symbols from the possibly-to-be-removed
header. That's hard to do without using something like llvm's
libclang. The one case that's perhaps a bit easier to automate, and
possibly worthwhile: If a header is not indirectly included (possibly
testable via #ifndef HEADER_NAME_H #error 'already included' etc), and
compilation doesn't fail with it removed, *then* it's actually likely
useless (except for portability cases).

* I think a lot of the interlinking stems from the bad idea to use
typedef's everywhere. In contrast to structs they cannot be forward
declared portably in our version of C. We should use a lot more struct
forward declarations, and just not use the typedef.

Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: Unused header file inclusion

Alvaro Herrera-9
On 2019-Jul-31, Andres Freund wrote:

> IDK, I find the compilation times annoying. And it's gotten quite
> noticably worse with all the speculative execution mitigations. Although
> to some degree that's not really the fault of individual compilations,
> but our buildsystem being pretty slow.

We're in a much better position now than a decade ago, in terms of clock
time.  Back then I would resort to many tricks to avoid spurious
compiles, even manually touching some files to dates in the past to
avoid them.  Nowadays I never bother with such things.  But yes,
reducing the build time even more would be welcome for sure.

> * I think a lot of the interlinking stems from the bad idea to use
> typedef's everywhere. In contrast to structs they cannot be forward
> declared portably in our version of C. We should use a lot more struct
> forward declarations, and just not use the typedef.

I don't know about that ... I think the problem is that we both declare
the typedef *and* define the struct in the same place.  If we were to
split those things to separate files, the required rebuilds would be
much less, I think, because changing a struct would no longer require
recompiles of files that merely pass those structs around (that's very
common for Node-derived structs).  Forward-declaring structs in
unrelated header files just because they need them, feels a bit like
cheating to me.

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: Unused header file inclusion

Tom Lane-2
Alvaro Herrera <[hidden email]> writes:
> On 2019-Jul-31, Andres Freund wrote:
>> * I think a lot of the interlinking stems from the bad idea to use
>> typedef's everywhere. In contrast to structs they cannot be forward
>> declared portably in our version of C. We should use a lot more struct
>> forward declarations, and just not use the typedef.

> I don't know about that ... I think the problem is that we both declare
> the typedef *and* define the struct in the same place.  If we were to
> split those things to separate files, the required rebuilds would be
> much less, I think, because changing a struct would no longer require
> recompiles of files that merely pass those structs around (that's very
> common for Node-derived structs).  Forward-declaring structs in
> unrelated header files just because they need them, feels a bit like
> cheating to me.

Yeah.  I seem to recall a proposal that nodes.h should contain

        typedef struct Foo Foo;

for every node type Foo, and then the other headers would just
fill in the structs, and we could get rid of a lot of ad-hoc
forward struct declarations and other hackery.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: Unused header file inclusion

Andres Freund
Hi,

On 2019-07-31 16:55:31 -0400, Tom Lane wrote:

> Alvaro Herrera <[hidden email]> writes:
> > On 2019-Jul-31, Andres Freund wrote:
> >> * I think a lot of the interlinking stems from the bad idea to use
> >> typedef's everywhere. In contrast to structs they cannot be forward
> >> declared portably in our version of C. We should use a lot more struct
> >> forward declarations, and just not use the typedef.
>
> > I don't know about that ... I think the problem is that we both declare
> > the typedef *and* define the struct in the same place.  If we were to
> > split those things to separate files, the required rebuilds would be
> > much less, I think, because changing a struct would no longer require
> > recompiles of files that merely pass those structs around (that's very
> > common for Node-derived structs).  Forward-declaring structs in
> > unrelated header files just because they need them, feels a bit like
> > cheating to me.
>
> Yeah.  I seem to recall a proposal that nodes.h should contain
>
> typedef struct Foo Foo;
>
> for every node type Foo, and then the other headers would just
> fill in the structs, and we could get rid of a lot of ad-hoc
> forward struct declarations and other hackery.

That to me just seems objectively worse. Now adding a new struct as a
minor implementation detail of some subsystem doesn't just require
recompiling the relevant files, but just about all of pg. And just about
every header would acquire a nodes.h include - there's still a lot of them
that today don't.

I don't understand why you guys consider forward declaring structs
ugly. It's what just about every other C project does. The only reason
it's sometimes problematic in postgres is that we "hide the pointer"
within some typedefs, making it not as obvious which type we're
referring to (because the struct usage will be struct FooData*, instead
of just Foo). But we also have confusion due to that in a lot of other
places, so I don't really buy that this is a significant issue.


Right now we really have weird dependencies between largely independent
subsystem. Some are partially because functions aren't always in the
right file, but it's also not often clear what the right one would
be. E.g. snapmgr.h depending on relcache.h (for
TransactionIdLimitedForOldSnapshots() having a Relation parameter), on
resowner.h (for RegisterSnapshotOnOwner()) is imo not good.  For one
they lead to a number of .c files that actually use functionality from
resowner.h to not have the necessary includes.  There's a lot of things
like that.

.oO(the fmgr.h include in snapmgr.h has been unnecessary since 352a24a1f9)


We could of course be super rigorous and have an accompanying
foo_structs.h or something for every foo.h. But that seems to add no
actual advantages, and makes things more complicated.


The only reason the explicit forward declaration is needed in the common
case of a 'struct foo*' parameter is that C has weird visibility rules
about the scope of forward declarations in paramters. If you instead
first have e.g. a function *return* type of that struct type, the
explicit forward declaration isn't even needed - it's visible
afterwards. But for parameters it's basically a *different* struct, that
cannot be referenced again. Note that in C++ the visibility routines are
more consistent, and you don't need an explicit forward declaration in
either case (I'd also be OK with requiring it in both cases, it's just
weird to only need them in one).

Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: Unused header file inclusion

Tom Lane-2
Andres Freund <[hidden email]> writes:
> On 2019-07-31 16:55:31 -0400, Tom Lane wrote:
>> Yeah.  I seem to recall a proposal that nodes.h should contain
>>
>> typedef struct Foo Foo;
>>
>> for every node type Foo, and then the other headers would just
>> fill in the structs, and we could get rid of a lot of ad-hoc
>> forward struct declarations and other hackery.

> That to me just seems objectively worse. Now adding a new struct as a
> minor implementation detail of some subsystem doesn't just require
> recompiling the relevant files, but just about all of pg.

Er, what?  This list of typedefs would change exactly when enum NodeTag
changes, so AFAICS your objection is bogus.

It's true that this proposal doesn't help for structs that aren't Nodes,
but my sense is that > 90% of our ad-hoc struct references are for Nodes.

> Right now we really have weird dependencies between largely independent
> subsystem.

Agreed, but I think fixing that will take some actually serious design
work.  It's not going to mechanically fall out of changing typedef rules.

> The only reason the explicit forward declaration is needed in the common
> case of a 'struct foo*' parameter is that C has weird visibility rules
> about the scope of forward declarations in paramters.

Yeah, but there's not much we can do about that, nor is getting rid
of typedefs in favor of "struct" going to make it even a little bit
better.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: Unused header file inclusion

Andres Freund
Hi,

On 2019-07-31 19:25:01 -0400, Tom Lane wrote:

> Andres Freund <[hidden email]> writes:
> > On 2019-07-31 16:55:31 -0400, Tom Lane wrote:
> >> Yeah.  I seem to recall a proposal that nodes.h should contain
> >>
> >> typedef struct Foo Foo;
> >>
> >> for every node type Foo, and then the other headers would just
> >> fill in the structs, and we could get rid of a lot of ad-hoc
> >> forward struct declarations and other hackery.
>
> > That to me just seems objectively worse. Now adding a new struct as a
> > minor implementation detail of some subsystem doesn't just require
> > recompiling the relevant files, but just about all of pg.
>
> Er, what?  This list of typedefs would change exactly when enum NodeTag
> changes, so AFAICS your objection is bogus.

> It's true that this proposal doesn't help for structs that aren't Nodes,
> but my sense is that > 90% of our ad-hoc struct references are for Nodes.

Ah, well, I somehow assumed you were talking about all nodes. I don't
think I agree with the 90% figure. In headers I feel like most the
references are to things like Relation, Snapshot, HeapTuple, etc.


> > Right now we really have weird dependencies between largely independent
> > subsystem.
>
> Agreed, but I think fixing that will take some actually serious design
> work.  It's not going to mechanically fall out of changing typedef rules.

No, but without finding a more workable approach than what we're doing
often doing now wrt includes and forward declares, we'll have a lot
harder time to separate subsystems out more.


> > The only reason the explicit forward declaration is needed in the common
> > case of a 'struct foo*' parameter is that C has weird visibility rules
> > about the scope of forward declarations in paramters.
>
> Yeah, but there's not much we can do about that, nor is getting rid
> of typedefs in favor of "struct" going to make it even a little bit
> better.

It imo pretty fundamentally does. You cannot redefine typedefs, but you
can forward declare structs.


E.g. in the attached series of patches, I'm removing a good portion of
unnecessary dependencies to fmgr.h. But to actually make a difference
that requires referencing two structs without including the header - and
I don't think restructing fmgr.h into two headers is a particularly
attractive alternative (would make it a lot more work and a lot more
invasive).

Think the first three are pretty clearly a good idea, I'm a bit less
sanguine about the fourth:
Headers like utils/timestamp.h are often included just because we need a
TimestampTz type somewhere, or call GetCurrentTimestamp(). Approximately
none of these need the PG_GETARG_* macros, which are the only reason for
including fmgr.h in these headers.  As they're macros that's not
actually needed, although I think normally good style. But I' think here
avoiding exposing fmgr.h to more headers is a bigger win.

Greetings,

Andres Freund

v1-0001-Remove-redundant-prototypes-for-SQL-callable-func.patch (1K) Download Attachment
v1-0002-Don-t-include-utils-array.h-from-acl.h.patch (7K) Download Attachment
v1-0003-Remove-fmgr.h-includes-from-headers-that-don-t-re.patch (11K) Download Attachment
v1-0004-Remove-fmgr.h-includes-from-headers-that-have-mac.patch (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Unused header file inclusion

vignesh C
In reply to this post by Andres Freund
On Wed, Jul 31, 2019 at 12:26 PM Andres Freund <[hidden email]> wrote:

>
> Hi,
>
> On 2019-07-31 11:19:08 +0530, vignesh C wrote:
> > I noticed that there are many header files being
> > included which need not be included.
> > I have tried this in a few files and found the
> > compilation and regression to be working.
> > I have attached the patch for the files that
> > I tried.
> > I tried this in CentOS, I did not find the header
> > files to be platform specific.
> > Should we pursue this further and cleanup in
> > all the files?
>
> These type of changes imo have a good chance of making things more
> fragile. A lot of the includes in header files are purely due to needing
> one or two definitions (which often could even be avoided by forward
> declarations). If you remove all the includes directly from the c files
> that are also included from some .h file, you increase the reliance on
> the indirect includes - making it harder to clean up.
>
> If anything, we should *increase* the number of includes, so we don't
> rely on indirect includes.  But that's also not necessarily the right
> choice, because it adds unnecessary dependencies.
>
>
> > --- a/src/backend/utils/mmgr/freepage.c
> > +++ b/src/backend/utils/mmgr/freepage.c
> > @@ -56,7 +56,6 @@
> >  #include "miscadmin.h"
> >
> >  #include "utils/freepage.h"
> > -#include "utils/relptr.h"
>
> I don't think it's a good idea to remove this header, for example. A
> *lot* of code in freepage.c relies on it. The fact that freepage.h also
> includes it here is just due to needing other parts of it
>
Fixed this.

>
> >  /* Magic numbers to identify various page types */
> > diff --git a/src/backend/utils/mmgr/portalmem.c b/src/backend/utils/mmgr/portalmem.c
> > index 334e35b..67268fd 100644
> > --- a/src/backend/utils/mmgr/portalmem.c
> > +++ b/src/backend/utils/mmgr/portalmem.c
> > @@ -26,7 +26,6 @@
> >  #include "utils/builtins.h"
> >  #include "utils/memutils.h"
> >  #include "utils/snapmgr.h"
> > -#include "utils/timestamp.h"
>
> Similarly, this file uses timestamp.h functions directly. The fact that
> timestamp.h already is included is just due to implementation details of
> xact.h that this file shouldn't depend on.
>
Fixed this.

Made the fixes based on your comments, updated patch has the changes
for the same.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

v2-0001-Unused-header-file-removal.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Unused header file inclusion

Alvaro Herrera-9
On 2019-Aug-04, vignesh C wrote:

> Made the fixes based on your comments, updated patch has the changes
> for the same.

Well, you fixed the two things that seem to me quoted as examples of
problems, but you didn't fix other occurrences of the same issues
elsewhere.  For example, you remove lwlock.h from dsa.c but there are
structs there that have LWLocks as members.  That's just the first hunk
of the patch; didn't look at the others but it wouldn't surprise that
they have similar issues.  I suggest this patch should be rejected.

Then there's the <limits.h> removal, which is in tuplesort.c because of
INT_MAX as added by commit d26559dbf356 and still present ...

FWIW sharedtuplestore.c, a very young file, also includes <limits.h> but
that appears to be copy-paste of includes from some other file (and also
in an inappropriate place), so I have no objections to obliterating that
one.  But other than that one line, this patch needs more "adult
supervision".

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: Unused header file inclusion

Tom Lane-2
Alvaro Herrera <[hidden email]> writes:
> Then there's the <limits.h> removal, which is in tuplesort.c because of
> INT_MAX as added by commit d26559dbf356 and still present ...

One has to be especially wary of removing system-header inclusions;
the fact that they don't seem to be needed on your own machine doesn't
prove they aren't needed elsewhere.

> ... I suggest this patch should be rejected.

Yeah.  If we do anything along this line it should be based on
pgrminclude results, and even then I think it'd require manual
review, especially for changes in header files.

The big picture here is that removing #includes is seldom worth
the effort it takes :-(

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: Unused header file inclusion

Alvaro Herrera-9
On 2019-Aug-05, Tom Lane wrote:

> Alvaro Herrera <[hidden email]> writes:
> > Then there's the <limits.h> removal, which is in tuplesort.c because of
> > INT_MAX as added by commit d26559dbf356 and still present ...
>
> One has to be especially wary of removing system-header inclusions;
> the fact that they don't seem to be needed on your own machine doesn't
> prove they aren't needed elsewhere.

As far as I can see, this line was just carried over from tuplestore.c,
but that file uses INT_MAX and this one doesn't use any of those macros
as far as I can tell.

I pushed this change and hereby consider this to be over.

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: Unused header file inclusion

Thomas Munro-5
On Thu, Aug 8, 2019 at 9:00 AM Alvaro Herrera <[hidden email]> wrote:

> On 2019-Aug-05, Tom Lane wrote:
> > Alvaro Herrera <[hidden email]> writes:
> > > Then there's the <limits.h> removal, which is in tuplesort.c because of
> > > INT_MAX as added by commit d26559dbf356 and still present ...
> >
> > One has to be especially wary of removing system-header inclusions;
> > the fact that they don't seem to be needed on your own machine doesn't
> > prove they aren't needed elsewhere.
>
> As far as I can see, this line was just carried over from tuplestore.c,
> but that file uses INT_MAX and this one doesn't use any of those macros
> as far as I can tell.

Yeah, probably, or maybe I used one of those sorts of macros in an
earlier version of the patch.

By the way, I see now that I had put the offending #include <limits.h>
*after* project headers, which is a convention I picked up from other
projects, mentors and authors, but not what PostgreSQL usually does.
In my own code I do that to maximise the chances that project headers
will fail to compile if they themselves forget to include the system
headers they depend on.  Of course an attempt to compile every header
in the project individually as a transaction unit also achieves that.

/me runs away

--
Thomas Munro
https://enterprisedb.com


12