Обсуждение: include-file cleanup
It took a little work to recompile after the include-file cleanups :-(.
You got overenthusiastic about removing #includes, apparently.
I have checked in changes for the ones that caused compile failures
or warnings here, but there may be more.
One thing that particularly disturbs me is that "config.h" was removed
from some of the files in src/backend/port/. I had to put this back
in the ones used on my platform. I didn't touch anything I didn't get
a warning from, but I would strongly counsel making sure that config.h
still gets included *everywhere*, even if it doesn't seem necessary.
Failing to do so may cause subtle problems due to #define symbols not
being defined where they should be or prototypes not being visible that
should be. This is especially dangerous in platform-specific code.
regards, tom lane
> It took a little work to recompile after the include-file cleanups :-(. > You got overenthusiastic about removing #includes, apparently. > I have checked in changes for the ones that caused compile failures > or warnings here, but there may be more. I have reviewed and replaced config.h in all files it appeared in in 6.5, where postgres.h or c.h were not already included. I have also removed config.h from the cleaning script, just as postgres.h was never removed. I imagine running this only every year or two. > One thing that particularly disturbs me is that "config.h" was removed > from some of the files in src/backend/port/. I had to put this back > in the ones used on my platform. I didn't touch anything I didn't get > a warning from, but I would strongly counsel making sure that config.h > still gets included *everywhere*, even if it doesn't seem necessary. I put them all back in the port/ directory. This is also the reason I did not change any of the system includes, because different OS's need different includes. > Failing to do so may cause subtle problems due to #define symbols not > being defined where they should be or prototypes not being visible that > should be. This is especially dangerous in platform-specific code. Yes, I know I fixed a number of places where tests were made before postgres.h/c.h/config.h were included. -- Bruce Momjian | http://www.op.net/~candle maillist@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
> I have reviewed and replaced config.h in all files it appeared in in
> 6.5, where postgres.h or c.h were not already included. I have also
> removed config.h from the cleaning script, just as postgres.h was never
> removed.
OK, that sounds good.
The thing that bothers me is why config.h got removed from these
port files in the first place. The compiler warning I got (because
I use gcc -Wmissing-prototypes) was that "random" and "srandom"
were defined without having been declared in any include file.
Now config.h provides prototypes for those functions --- inside
#ifdefs of course, but they are there. Your script should have
noticed that the name "random" mentioned in config.h was also
mentioned in port/random.c, and therefore not removed the include
of config.h from random.c. Why did it not make the connection?
regards, tom lane
> > I have reviewed and replaced config.h in all files it appeared in in > > 6.5, where postgres.h or c.h were not already included. I have also > > removed config.h from the cleaning script, just as postgres.h was never > > removed. > > OK, that sounds good. > > The thing that bothers me is why config.h got removed from these > port files in the first place. The compiler warning I got (because > I use gcc -Wmissing-prototypes) was that "random" and "srandom" > were defined without having been declared in any include file. > Now config.h provides prototypes for those functions --- inside > #ifdefs of course, but they are there. Your script should have > noticed that the name "random" mentioned in config.h was also > mentioned in port/random.c, and therefore not removed the include > of config.h from random.c. Why did it not make the connection? Because the random prototype is in stdlib.h in BSD/OS, and that file was already #included. Seems it must be in another file in your OS. stdlib.h:168: long random __P((void)); -- Bruce Momjian | http://www.op.net/~candle maillist@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
Bruce Momjian <maillist@candle.pha.pa.us> writes:
>> Your script should have
>> noticed that the name "random" mentioned in config.h was also
>> mentioned in port/random.c, and therefore not removed the include
>> of config.h from random.c. Why did it not make the connection?
> Because the random prototype is in stdlib.h in BSD/OS, and that file was
> already #included. Seems it must be in another file in your OS.
> stdlib.h:168: long random __P((void));
Ah, well there is the problem: you are (in effect) assuming that
if stdlib.h defines random() on your platform, then it does so on
everyone's platform. If that were true, we'd not need port/random.c...
I think your script ought to be set up to ignore system headers
completely, and only look at our own headers to determine which
symbols are defined where.
In theory, only config.h and c.h should contain any substitutes for
system-header symbols, so if you explicitly exclude those two from
removal then there should be no portability issue. But I'd be
happier if you changed the script so that its decisions are not
affected by the contents of system headers.
regards, tom lane
> Bruce Momjian <maillist@candle.pha.pa.us> writes: > >> Your script should have > >> noticed that the name "random" mentioned in config.h was also > >> mentioned in port/random.c, and therefore not removed the include > >> of config.h from random.c. Why did it not make the connection? > > > Because the random prototype is in stdlib.h in BSD/OS, and that file was > > already #included. Seems it must be in another file in your OS. > > stdlib.h:168: long random __P((void)); > > Ah, well there is the problem: you are (in effect) assuming that > if stdlib.h defines random() on your platform, then it does so on > everyone's platform. If that were true, we'd not need port/random.c... > > I think your script ought to be set up to ignore system headers > completely, and only look at our own headers to determine which > symbols are defined where. Well, the script just does the compile with and without the #include. Really no way to test if the existance of system tables causes any difference, and if I remove the system files completely, the code will not compile. > In theory, only config.h and c.h should contain any substitutes for > system-header symbols, so if you explicitly exclude those two from > removal then there should be no portability issue. But I'd be > happier if you changed the script so that its decisions are not > affected by the contents of system headers. I have added c.h to the list of files I skip in pgnoinclude. I can't think of any way to prevent system headers from causing such problems, though. I specifically don't remove any system headers for this very reason, that different OS's may need system files that I don't. -- Bruce Momjian | http://www.op.net/~candle maillist@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
Bruce Momjian <maillist@candle.pha.pa.us> writes:
> Well, the script just does the compile with and without the #include.
Oh ... I was assuming you had built something that actually went through
and gathered up a list of the symbols mentioned in each file.
I fear we are going to be putting back missing includes for a while to
come; in particular, I'll bet that MULTIBYTE and possibly USE_LOCALE are
now broken, unless you ran the script with those features enabled.
There are going to be a few more problems with platform-specific code
like the one I found in pqcomm.c, too.
What you did is a good hack as a one-shot cleanup, but I can't see
wanting to repeat it in future, not even as seldom as every year or two,
unless we build a much more reliable tool for the job.
regards, tom lane
> Bruce Momjian <maillist@candle.pha.pa.us> writes: > > Well, the script just does the compile with and without the #include. > > Oh ... I was assuming you had built something that actually went through > and gathered up a list of the symbols mentioned in each file. > > I fear we are going to be putting back missing includes for a while to > come; in particular, I'll bet that MULTIBYTE and possibly USE_LOCALE are > now broken, unless you ran the script with those features enabled. > There are going to be a few more problems with platform-specific code > like the one I found in pqcomm.c, too. I have added the needed files for MULTIBYTE and LOCALE. > > What you did is a good hack as a one-shot cleanup, but I can't see > wanting to repeat it in future, not even as seldom as every year or two, > unless we build a much more reliable tool for the job. OK, I will not run it for another three years. Some of the tools, like the one that changes <> to "" as approproate may be good for more frequent use. The tool that makes sure every #include has proper includes may be OK too. As you suggested, if we run the include removal script, we can just have it report what it suggests for removal, and manually review each one. -- Bruce Momjian | http://www.op.net/~candle maillist@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
A further thought on the include-file cleanup: although you are right to
be wary of removing includes of system files, it would be a good idea to
remove *redundant* includes of system files.
In particular, since c.h includes <stdlib.h>, as well as <stddef.h>
and <stdarg.h> if they exist, it should not be necessary for any file
that includes c.h (either directly or via postgres.h) to pull these
in for itself. Removing the "retail" inclusions of these files that
are found in many source files would make life much easier for anyone
trying to port to a platform where they don't exist...
Also, I think some places include c.h without having included
postgres.h. These should be checked to ensure that config.h has
been included first --- c.h depends on configuration symbols from
config.h to work properly.
regards, tom lane
> A further thought on the include-file cleanup: although you are right to > be wary of removing includes of system files, it would be a good idea to > remove *redundant* includes of system files. > > In particular, since c.h includes <stdlib.h>, as well as <stddef.h> > and <stdarg.h> if they exist, it should not be necessary for any file > that includes c.h (either directly or via postgres.h) to pull these > in for itself. Removing the "retail" inclusions of these files that > are found in many source files would make life much easier for anyone > trying to port to a platform where they don't exist... The problem is that we include system includes first. Are there any system includes that require stdlib to be included first? OK, now you got me started again. And I was going to do some real paying work today. :-) I have removed the duplicate system headers when postgres.h is included, and have added string.h and stdio.h to c.h, and have removed those from the files. Now, many C files have _no_ system includes, because they come from postgres.h including c.h. You know, at the time, this seems like a real pain, just like pgindent was a pain to get working properly. But in a year, you look back and say, "Hey, it was worth it. Look at how much easier things are now." I will commit the changes now. > Also, I think some places include c.h without having included > postgres.h. These should be checked to ensure that config.h has > been included first --- c.h depends on configuration symbols from > config.h to work properly. postgres.h include c.h, and config.h _now_ includes c.h. -- Bruce Momjian | http://www.op.net/~candle maillist@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
Bruce Momjian <maillist@candle.pha.pa.us> writes:
> The problem is that we include system includes first. Are there any
> system includes that require stdlib to be included first?
If so, they are supposed to include it for themselves.
Note: you can't really include ALL sys headers first, since some of them
need to be included conditionally, and the condition symbols are coming
from config.h...
> I have removed the duplicate system headers when postgres.h is included,
> and have added string.h and stdio.h to c.h, and have removed those from
> the files. Now, many C files have _no_ system includes, because they
> come from postgres.h including c.h.
Sounds pretty good.
>> Also, I think some places include c.h without having included
>> postgres.h. These should be checked to ensure that config.h has
>> been included first --- c.h depends on configuration symbols from
>> config.h to work properly.
>
> postgres.h include c.h, and config.h _now_ includes c.h.
OK, so then no .c files should be including c.h directly anymore?
Everything should include either postgres.h, or config.h if it's
not tightly tied to the system?
regards, tom lane
> > postgres.h include c.h, and config.h _now_ includes c.h. > > OK, so then no .c files should be including c.h directly anymore? > Everything should include either postgres.h, or config.h if it's > not tightly tied to the system? Some port stuff includes just c.h. Not sure why. -- Bruce Momjian | http://www.op.net/~candle maillist@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
I wrote:
>> OK, so then no .c files should be including c.h directly anymore?
>> Everything should include either postgres.h, or config.h if it's
>> not tightly tied to the system?
I misread your prior mail --- you have fixed c.h to include
config.h, so it's safe to include c.h directly if not including
postgres.h. The real story now is: either c.h or postgres.h should be
the first file included by all .c files in Postgres, to ensure that the
autoconfigure symbols from config.h are picked up.
Bruce Momjian <maillist@candle.pha.pa.us> writes:
> Some port stuff includes just c.h. Not sure why.
That makes sense to me for files that are just duplicating missing
system functions, and don't really need to be aware that they are
inside Postgres.
Looks like things are in good shape now. I will run a compile here
momentarily...
regards, tom lane