Thread: [HACKERS] Removing #include "postgres.h" from a couple of headers
Hi, Over in another thread it was pointed out that a patch I submitted broke a project rule by including "postgres.h" in a header. Here is a patch to remove it from dsa.h where I made the same mistake, and also a case I found in bufmask.h by grepping. There are also instances in regcustom.h and snowball's header.h -- are those special cases? -- Thomas Munro http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
On Wed, Mar 8, 2017 at 5:55 PM, Thomas Munro <thomas.munro@enterprisedb.com> wrote: > Over in another thread it was pointed out that a patch I submitted > broke a project rule by including "postgres.h" in a header. Here is a > patch to remove it from dsa.h where I made the same mistake, and also > a case I found in bufmask.h by grepping. > > There are also instances in regcustom.h and snowball's header.h -- are > those special cases? --- a/src/include/access/bufmask.h +++ b/src/include/access/bufmask.h @@ -17,7 +17,6 @@#ifndef BUFMASK_H#define BUFMASK_H -#include "postgres.h"#include "storage/block.h"#include "storage/bufmgr.h" Oops. This really escaped me... -- Michael
On Wed, Mar 8, 2017 at 3:55 AM, Thomas Munro <thomas.munro@enterprisedb.com> wrote: > Over in another thread it was pointed out that a patch I submitted > broke a project rule by including "postgres.h" in a header. Here is a > patch to remove it from dsa.h where I made the same mistake, and also > a case I found in bufmask.h by grepping. Committed. > There are also instances in regcustom.h and snowball's header.h -- are > those special cases? I will leave this question to someone wiser (or more self-assured) than I. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Mar 8, 2017 at 3:55 AM, Thomas Munro
> <thomas.munro@enterprisedb.com> wrote:
>> There are also instances in regcustom.h and snowball's header.h -- are
>> those special cases?
> I will leave this question to someone wiser (or more self-assured) than I.
I'm pretty sure I'm to blame for both of those special cases. The genesis
of both is that we are including these headers from externally-generated
.c files, and it seemed like modifying the .c files would be a bigger
problem than violating the policy. I am not sure if I hold that position
anymore for the regexp library; our copy has diverged substantially from
Tcl's anyway. It's still an issue for Snowball, because those .c files
are actually machine-generated by a Snowball-to-C compiler. We haven't
modified them and probably shouldn't.
If we don't change the code layout, we should probably at least add
comments near these postgres.h inclusions explaining why they're violating
policy.
regards, tom lane
... BTW, a bit of grepping shows that there are still significant
violations of this policy with respect to header-driven inclusion of
postgres_fe.h and c.h. Also, plpgsql is doing it in the unapproved
fashion. Cleaning these up will take a bit more work, since we'll have
to actually add #includes to some .c files that are currently relying on
those headers to get the core header. However, if I believe my own
argument, then that's a good thing and we'd better go do it.
Also, ecpglib.h seems like a complete mess: it's relying on symbols
like ENABLE_NLS but there's no certainty as to whether pg_config.h
has been included first. It won't have been in the case where this
header is read from an ecpg-generated .c file. So this header will
in fact be interpreted differently in ecpg-generated programs than
in ecpglib and ecpg itself. Maybe this is okay but it sure smells
like trouble waiting to happen. I have no desire to try to fix it
myself though.
regards, tom lane