Unused header file inclusion

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

On Wed, Jul 31, 2019 at 11:26 AM Michael Paquier <michael@paquier.xyz> 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

On Wed, Jul 31, 2019 at 11:31 AM vignesh C <vignesh21@gmail.com> wrote:

On Wed, Jul 31, 2019 at 11:26 AM Michael Paquier <michael@paquier.xyz> 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]http://commitfest.cputube.org/ 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

On Wed, Jul 31, 2019 at 11:55 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Wed, Jul 31, 2019 at 11:31 AM vignesh C <vignesh21@gmail.com> wrote:

On Wed, Jul 31, 2019 at 11:26 AM Michael Paquier <michael@paquier.xyz> 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

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

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

Michael Paquier <michael@paquier.xyz> 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

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

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

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

Alvaro Herrera <alvherre@2ndquadrant.com> 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

Hi,

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

Alvaro Herrera <alvherre@2ndquadrant.com> 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

Andres Freund <andres@anarazel.de> 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

Hi,

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

Andres Freund <andres@anarazel.de> 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

On Wed, Jul 31, 2019 at 12:26 PM Andres Freund <andres@anarazel.de> 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

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

Alvaro Herrera <alvherre@2ndquadrant.com> 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

On 2019-Aug-05, Tom Lane wrote:

Alvaro Herrera <alvherre@2ndquadrant.com> 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

On Thu, Aug 8, 2019 at 9:00 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

On 2019-Aug-05, Tom Lane wrote:

Alvaro Herrera <alvherre@2ndquadrant.com> 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

On Thu, Aug 8, 2019 at 9:47 AM Thomas Munro <thomas.munro@gmail.com> wrote:

transaction unit

*translation unit

--
Thomas Munro
https://enterprisedb.com

Hi,

On 2019-08-03 12:37:33 -0700, Andres Freund wrote:

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.

I still think the fourth is probably worthwhile, but I don't feel
confident enough to do it without somebody else +0.5'ing it...

I've pushed the other ones.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

I've pushed the other ones.

Checking whether header files compile standalone shows you were overly
aggressive about removing fmgr.h includes:

In file included from /tmp/headerscheck.Ss8bVx/test.c:3:
./src/include/utils/selfuncs.h:143: error: expected declaration specifiers or '...' before 'FmgrInfo'
./src/include/utils/selfuncs.h:146: error: expected declaration specifiers or '...' before 'FmgrInfo'
./src/include/utils/selfuncs.h:152: error: expected declaration specifiers or '...' before 'FmgrInfo'

That's with a script I use that's like cpluspluscheck except it tests
with plain gcc not g++. I attached it for the archives' sake.

Oddly, cpluspluscheck does not complain about those cases, but it
does complain about

In file included from /tmp/cpluspluscheck.FgX2SW/test.cpp:4:
./src/bin/scripts/scripts_parallel.h:18: error: ISO C++ forbids declaration of 'PGconn' with no type
./src/bin/scripts/scripts_parallel.h:18: error: expected ';' before '*' token
./src/bin/scripts/scripts_parallel.h:29: error: 'PGconn' has not been declared

(My headerscheck script is missing that header; I need to update it to
match the latest version of cpluspluscheck.)

regards, tom lane

I wrote:

(My headerscheck script is missing that header; I need to update it to
match the latest version of cpluspluscheck.)

I did that, and ended up with the attached. I'm rather tempted to stick
this into src/tools/ alongside cpluspluscheck, because it seems to find
rather different trouble spots than cpluspluscheck does. Thoughts?

As of HEAD (927f34ce8), I get clean passes from both this and
cpluspluscheck, except for the FmgrInfo references in selfuncs.h.
I tried it on RHEL/Fedora, FreeBSD 12, and current macOS.

regards, tom lane

On 2019-Aug-18, Tom Lane wrote:

I wrote:

(My headerscheck script is missing that header; I need to update it to
match the latest version of cpluspluscheck.)

I did that, and ended up with the attached. I'm rather tempted to stick
this into src/tools/ alongside cpluspluscheck, because it seems to find
rather different trouble spots than cpluspluscheck does. Thoughts?

Yeah, let's include this. I've written its equivalent a couple of times
already. (My strategy is just to compile the .h file directly though,
which creates a .gch file, rather than writing a temp .c file.)

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

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

On 2019-Aug-18, Tom Lane wrote:

I did that, and ended up with the attached. I'm rather tempted to stick
this into src/tools/ alongside cpluspluscheck, because it seems to find
rather different trouble spots than cpluspluscheck does. Thoughts?

Yeah, let's include this.

Done.

regards, tom lane

Hi,

On 2019-08-18 14:37:34 -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

I've pushed the other ones.

Checking whether header files compile standalone shows you were overly
aggressive about removing fmgr.h includes:

In file included from /tmp/headerscheck.Ss8bVx/test.c:3:
./src/include/utils/selfuncs.h:143: error: expected declaration specifiers or '...' before 'FmgrInfo'
./src/include/utils/selfuncs.h:146: error: expected declaration specifiers or '...' before 'FmgrInfo'
./src/include/utils/selfuncs.h:152: error: expected declaration specifiers or '...' before 'FmgrInfo'

Darn. Pushed the obvious fix of adding a direct fmgr.h include, rather
than the preivous indirect include.

That's with a script I use that's like cpluspluscheck except it tests
with plain gcc not g++. I attached it for the archives' sake.

Oddly, cpluspluscheck does not complain about those cases, but it
does complain about

Hm. I don't understand why it's not complaining. Wonder if it's a
question of the flags or such.

In file included from /tmp/cpluspluscheck.FgX2SW/test.cpp:4:
./src/bin/scripts/scripts_parallel.h:18: error: ISO C++ forbids declaration of 'PGconn' with no type
./src/bin/scripts/scripts_parallel.h:18: error: expected ';' before '*' token
./src/bin/scripts/scripts_parallel.h:29: error: 'PGconn' has not been declared

I noticed that "manually" earlier, when looking at the openbsd issue.

(My headerscheck script is missing that header; I need to update it to
match the latest version of cpluspluscheck.)

I wonder if we should just add a #ifndef HEADERCHECK or such to the
headers that we don't want to process as standalone headers (or #ifndef
NOT_STANDALONE or whatever). That way multiple tools can rely on these markers,
rather than copying knowledge about that kind of information into
multiple places.

I wish we could move the whole logic of those scripts into makefiles, so
we could employ parallelism.

Greetings,

Andres Freund

Hi,

On 2019-08-19 13:07:50 -0700, Andres Freund wrote:

On 2019-08-18 14:37:34 -0400, Tom Lane wrote:

That's with a script I use that's like cpluspluscheck except it tests
with plain gcc not g++. I attached it for the archives' sake.

Oddly, cpluspluscheck does not complain about those cases, but it
does complain about

Hm. I don't understand why it's not complaining. Wonder if it's a
question of the flags or such.

I'ts caused by -fsyntax-only

# These switches are g++ specific, you may override if necessary.
CXXFLAGS=${CXXFLAGS:- -fsyntax-only -Wall}

which also explains why your headercheck doesn't have that problem, it
doesn't use -fsyntax-only.

(My headerscheck script is missing that header; I need to update it to
match the latest version of cpluspluscheck.)

I wonder if we should just add a #ifndef HEADERCHECK or such to the
headers that we don't want to process as standalone headers (or #ifndef
NOT_STANDALONE or whatever). That way multiple tools can rely on these markers,
rather than copying knowledge about that kind of information into
multiple places.

I wish we could move the whole logic of those scripts into makefiles, so
we could employ parallelism.

Hm. Perhaps the way to do that would be to use gcc's -include to include
postgres.h, and use -Wc++-compat to detect c++ issues, rather than using
g++. Without tempfiles it ought to be a lot easier to just do all of the
relevant work in make, without a separate shell script. The
python/perl/ecpg logic would b e abit annoying, but probably not too
bad? Think we could just always add all of them?

Greetings,

Andres Freund

On 2019-Aug-19, Andres Freund wrote:

I wish we could move the whole logic of those scripts into makefiles, so
we could employ parallelism.

Hm. Perhaps the way to do that would be to use gcc's -include to include
postgres.h, and use -Wc++-compat to detect c++ issues, rather than using
g++. Without tempfiles it ought to be a lot easier to just do all of the
relevant work in make, without a separate shell script.

I used to have this:
/messages/by-id/1293469595-sup-1462@alvh.no-ip.org
Not sure how much this helps, since it's a shell line in make, so not
very paralellizable. And you still have to build the exclusions
somehow.

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

Andres Freund <andres@anarazel.de> writes:

On 2019-08-19 13:07:50 -0700, Andres Freund wrote:

On 2019-08-18 14:37:34 -0400, Tom Lane wrote:

Oddly, cpluspluscheck does not complain about those cases, but it
does complain about

Hm. I don't understand why it's not complaining. Wonder if it's a
question of the flags or such.

I'ts caused by -fsyntax-only

Ah-hah. Should we change that to something else? That's probably
a hangover from thinking that all we had to do was check for C++
keywords.

I wish we could move the whole logic of those scripts into makefiles, so
we could employ parallelism.

I can't really get excited about expending a whole bunch of additional
work on these scripts.

regards, tom lane