Thread: C++ keywords in headers (was Re: [GENERAL] #include )
Craig Ringer <craig@postnewspapers.com.au> writes:
> On 12/26/2010 02:14 PM, Elliot Chance wrote:
>> /usr/include/pgsql/server/nodes/primnodes.h:1155: error: expected unqualified-id before �using�
> You've neglected to mention which version of Pg you're compiling
> against, so that line number means nothing. What's the offending line of
> code and the surrounding few lines in primnodes.h?
I'm betting 8.4, because there is a field named "using" on that line in
8.4 ...
> In any case, I think it's very likely the issue is a C/C++
> incompatibility in the Pg headers. It fails for me in a different place
> using Pg 9.1git and g++ 4.5, complaining about the use of "private" as
> an identifier in fmgr.h, because it's a keyword in C++.
We did clean up C++ keyword uses in the header files in 9.0, but your
report shows it's already gotten broken again. I'm disinclined to fix
it unless someone steps up to create an automated test that will get run
reasonably often. We had that discussion when the patch to rename
keyword-named fields was proposed, and nothing got done, and the current
state of affairs is the entirely predictable result.
regards, tom lane
On 12/26/2010 12:31 PM, Tom Lane wrote:
>
>> In any case, I think it's very likely the issue is a C/C++
>> incompatibility in the Pg headers. It fails for me in a different place
>> using Pg 9.1git and g++ 4.5, complaining about the use of "private" as
>> an identifier in fmgr.h, because it's a keyword in C++.
> We did clean up C++ keyword uses in the header files in 9.0, but your
> report shows it's already gotten broken again. I'm disinclined to fix
> it unless someone steps up to create an automated test that will get run
> reasonably often. We had that discussion when the patch to rename
> keyword-named fields was proposed, and nothing got done, and the current
> state of affairs is the entirely predictable result.
Here's a script to play with. It needs some tuning but it's a start. Run
in the top level directory (where configure etc live).
Sample output:
found new in src/timezone/private.h
found private in src/include/fmgr.h
found not in src/include/libpq/pqcomm.h
found typeid in src/include/access/htup.h
found bitor, bitand in src/include/catalog/pg_operator.h
found bitor, bitand in src/include/catalog/pg_proc.h
found bitand in src/include/catalog/pg_aggregate.h
found using in src/include/catalog/indexing.h
found inline in src/include/nodes/pg_list.h
found or in src/include/port/win32.h
found or in src/include/port/cygwin.h
found typeid in src/include/parser/parse_type.h
found inline in src/include/portability/instr_time.h
found inline in src/include/utils/palloc.h
found bitor, bitand in src/include/utils/varbit.h
found wchar_t in src/include/mb/pg_wchar.h
found inline, not, using, this, asm in src/include/storage/s_lock.h
found new in src/interfaces/ecpg/preproc/type.h
found new in src/pl/plpgsql/src/plpgsql.h
found namespace in src/bin/pg_dump/pg_dump.h
found namespace, public in src/bin/pg_dump/pg_backup_archiver.h
found namespace in src/bin/pg_dump/pg_backup.h
cheers
andrew
Attachment
On sön, 2010-12-26 at 12:31 -0500, Tom Lane wrote: > We did clean up C++ keyword uses in the header files in 9.0, but your > report shows it's already gotten broken again. I'm disinclined to fix > it unless someone steps up to create an automated test that will get > run reasonably often. We had that discussion when the patch to rename > keyword-named fields was proposed, and nothing got done, and the > current state of affairs is the entirely predictable result. src/tools/pginclude/cpluspluscheck What's missing is to automate this, but it's unclear in what context, and perhaps also to what extend this should be a hard requirement.
Excerpts from Peter Eisentraut's message of lun dic 27 12:54:16 -0300 2010: > On sön, 2010-12-26 at 12:31 -0500, Tom Lane wrote: > > We did clean up C++ keyword uses in the header files in 9.0, but your > > report shows it's already gotten broken again. I'm disinclined to fix > > it unless someone steps up to create an automated test that will get > > run reasonably often. We had that discussion when the patch to rename > > keyword-named fields was proposed, and nothing got done, and the > > current state of affairs is the entirely predictable result. > > src/tools/pginclude/cpluspluscheck > > What's missing is to automate this, but it's unclear in what context, > and perhaps also to what extend this should be a hard requirement. Maybe we could mention it in src/tools/RELEASE_CHANGES -- Álvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Peter Eisentraut <peter_e@gmx.net> writes:
> src/tools/pginclude/cpluspluscheck
Ah, I'd forgotten that.
> What's missing is to automate this, but it's unclear in what context,
> and perhaps also to what extend this should be a hard requirement.
After a bit of experimentation, I can say that this is better than
Andrew's hack, but it's still a good distance shy of something that
should be automated or treated as a hard requirement. The problem with
it is that it doesn't know anything about inclusion-order restrictions;
and to make matters worse, the actual order in which things are included
will vary for different users, because of the unspecified order in which
'find' will find things.
[ lightbulb ] ... although we could improve that quite a bit if we
processed each .h file separately instead of insisting on smashing
everything into one compilation. Let me go try that.
regards, tom lane
Excerpts from Tom Lane's message of lun dic 27 13:54:56 -0300 2010: > [ lightbulb ] ... although we could improve that quite a bit if we > processed each .h file separately instead of insisting on smashing > everything into one compilation. Let me go try that. FWIW I have this patch lingering about that I wrote months ago, to check for header problems (not C++ stuff, just things like forgetting to include some necessary header in some other header). Since it needs a lot of polish (needs to ignore certain headers, and avoid leave lingering files around), I didn't commit it; and I haven't updated it to the new Make recursive stuff, either. Maybe someone else knows what to do with it, though. *** a/src/include/Makefile --- b/src/include/Makefile *************** uninstall: *** 60,65 **** --- 60,72 ---- # heuristic... rm -rf $(addprefix '$(DESTDIR)$(includedir_server)'/, $(SUBDIRS) *.h) + check: + for dir in $(SUBDIRS); do \ + for header in `find $(srcdir)/$$dir -type f -name \*.h`; do \ + echo $$header; \ + $(CC) $(CFLAGS) $(CPPFLAGS) -include postgres.h -o $$dir/`basename $$header .h`.gch $$header; \ + done; \ + done clean: rm -f utils/fmgroids.h parser/gram.h utils/probes.h catalog/schemapg.h -- Álvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On 12/27/2010 11:54 AM, Tom Lane wrote: > After a bit of experimentation, I can say that this is better than > Andrew's hack, but it's still a good distance shy of something that > should be automated or treated as a hard requirement. I'm always happy if someone produces something better than I did :-) On a more general point, it would be useful to have some infrastructure for running quality checks like this and publishing the results. We should be way beyond the point where we rely on individuals doing this sort of stuff. cheers andrew
On Mon, Dec 27, 2010 at 12:33:00PM -0500, Andrew Dunstan wrote: > On 12/27/2010 11:54 AM, Tom Lane wrote: > >After a bit of experimentation, I can say that this is better than > >Andrew's hack, but it's still a good distance shy of something that > >should be automated or treated as a hard requirement. > > I'm always happy if someone produces something better than I did :-) > > On a more general point, it would be useful to have some > infrastructure for running quality checks like this and publishing > the results. We should be way beyond the point where we rely on > individuals doing this sort of stuff. This sounds like an excellent early candidate for the bitrot farm. Cheers, David. -- David Fetter <david@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
On mån, 2010-12-27 at 12:33 -0500, Andrew Dunstan wrote: > On a more general point, it would be useful to have some > infrastructure for running quality checks like this and publishing the > results. We should be way beyond the point where we rely on > individuals doing this sort of stuff. I had a Hudson service set up for things like this, but the hosting was unreliable and then the thing faded away. I could try to revive it.
Alvaro Herrera <alvherre@commandprompt.com> writes:
> Excerpts from Tom Lane's message of lun dic 27 13:54:56 -0300 2010:
>> [ lightbulb ] ... although we could improve that quite a bit if we
>> processed each .h file separately instead of insisting on smashing
>> everything into one compilation. Let me go try that.
> FWIW I have this patch lingering about that I wrote months ago, to check
> for header problems (not C++ stuff, just things like forgetting to
> include some necessary header in some other header). Since it needs a
> lot of polish (needs to ignore certain headers, and avoid leave
> lingering files around), I didn't commit it; and I haven't updated it to
> the new Make recursive stuff, either.
src/tools/pginclude/ already contains several scripts for this sort of
thing. Bruce runs them by hand occasionally, although I just found out
that he's evidently not run the does-each-header-compile-standalone
test in awhile. It would probably pay to automate these.
regards, tom lane
On Mon, Dec 27, 2010 at 18:50, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Alvaro Herrera <alvherre@commandprompt.com> writes: >> Excerpts from Tom Lane's message of lun dic 27 13:54:56 -0300 2010: >>> [ lightbulb ] ... although we could improve that quite a bit if we >>> processed each .h file separately instead of insisting on smashing >>> everything into one compilation. Let me go try that. > >> FWIW I have this patch lingering about that I wrote months ago, to check >> for header problems (not C++ stuff, just things like forgetting to >> include some necessary header in some other header). Since it needs a >> lot of polish (needs to ignore certain headers, and avoid leave >> lingering files around), I didn't commit it; and I haven't updated it to >> the new Make recursive stuff, either. > > src/tools/pginclude/ already contains several scripts for this sort of > thing. Bruce runs them by hand occasionally, although I just found out > that he's evidently not run the does-each-header-compile-standalone > test in awhile. It would probably pay to automate these. Could this at some point be platform dependent? If so, could it be run on the buildfarm? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Magnus Hagander <magnus@hagander.net> writes:
> On Mon, Dec 27, 2010 at 18:50, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> src/tools/pginclude/ already contains several scripts for this sort of
>> thing. �Bruce runs them by hand occasionally, although I just found out
>> that he's evidently not run the does-each-header-compile-standalone
>> test in awhile. �It would probably pay to automate these.
> Could this at some point be platform dependent? If so, could it be run
> on the buildfarm?
It's certainly possible that someone could use a C++ keyword in a
platform-specific, or feature-specific, part of an include file.
The current form of this script will only find problems in active
code; so running it on the buildfarm would be indicated if we want
to have real confidence that there are no problems.
regards, tom lane
BTW, the cpluspluscheck script invokes g++ with -fno-operator-names,
saying
# -fno-operator-names omits the definition of bitand and bitor, which# collide with varbit.h. Could be fixed, if one
wereso inclined.
I just confirmed that those two function definitions are the only issues
that currently show up if one removes the switch. Now, I'm not that
concerned about whether C++ users can include varbit.h ... but if we're
really going to use this technique to check whether C++ can include
headers, I think we've got to get rid of that switch, or we'll get
bitten elsewhere.
I propose renaming bitand() and bitor() to bit_and and bit_or() ...
any objections?
regards, tom lane
I hope that we don't make the mistake of not checking for collisions with C++0x keywords, for which GCC 4.3+ has partial support. The new standard is almost complete, so it will probably become a lot more relevant soon. There are quite a few new keywords in C++0x, including: constexpr decltype nullptr static_assert Perhaps we should add -std=c++0x to the g++ command in cpluspluscheck.sh . Since C++0x is “almost 100-percent compatible with the existing Standard C++” according to no less an authority than Bjarne Stroustrup, this seems sensible. -- Regards, Peter Geoghegan
Peter Geoghegan <peter.geoghegan86@gmail.com> writes:
> I hope that we don't make the mistake of not checking for collisions
> with C++0x keywords, for which GCC 4.3+ has partial support. The new
> standard is almost complete, so it will probably become a lot more
> relevant soon. There are quite a few new keywords in C++0x, including:
[ shrug... ] If it's not a keyword according to popularly available
tools, then I really have zero interest in worrying about it. This
is an exercise in making the headers useful in practice, not in academic
standards conformance.
regards, tom lane
On 27 December 2010 19:17, Tom Lane <tgl@sss.pgh.pa.us> wrote: > [ shrug... ] If it's not a keyword according to popularly available > tools, then I really have zero interest in worrying about it. This > is an exercise in making the headers useful in practice, not in academic > standards conformance. It isn't academic and I'm not just being pedantic, because the standard introduces many new, useful features. A lot of popular C++ libraries optionally use C++0x through the use of conditional compilation. For example, my distro's libstdc++ standard library (which is mostly header-based due to the fact that it is mostly comprised of templates and inline functions) has many #ifdefs, so that things like move constructors (a big performance win for standard library containers) are available. It just seems prudent to assume that if any of these pg headers are being included in C++ TUs, they might well be using C++0x. -- Regards, Peter Geoghegan
Excerpts from Peter Geoghegan's message of lun dic 27 16:13:33 -0300 2010: > I hope that we don't make the mistake of not checking for collisions > with C++0x keywords, for which GCC 4.3+ has partial support. The new > standard is almost complete, so it will probably become a lot more > relevant soon. There are quite a few new keywords in C++0x, including: > > constexpr > decltype > nullptr > static_assert I think only constexpr is being currently used from this list, and it's easily fixed because it's not exposed beyond a single file. -- Álvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@commandprompt.com> writes:
> Excerpts from Peter Geoghegan's message of lun dic 27 16:13:33 -0300 2010:
>> constexpr
>> decltype
>> nullptr
>> static_assert
> I think only constexpr is being currently used from this list, and it's
> easily fixed because it's not exposed beyond a single file.
If you're looking at the usage in predtest.c, it's not an issue since
it's not in a header file. There's no ambition here to make our .c
files compilable in C++.
regards, tom lane
peter_e@gmx.net (Peter Eisentraut) writes: > On mån, 2010-12-27 at 12:33 -0500, Andrew Dunstan wrote: >> On a more general point, it would be useful to have some >> infrastructure for running quality checks like this and publishing >> the results. We should be way beyond the point where we rely on >> individuals doing this sort of stuff. > > I had a Hudson service set up for things like this, but the hosting > was unreliable and then the thing faded away. I could try to revive > it. Careful, Oracle has been trying to claim proprietary ownership of that... <http://hudson-labs.org/content/whos-driving-thing> -- ``God decided to take the devil to court and settle their differences once and for all. When Satan heard of this, he grinned and said, "And just where do you think you're going to find a lawyer?"''
I believe that Dave Page wants to move to building pg for windows using visual C++ 2010 some time this year. That alone may be enough of a reason to check for C++0x keywords in headers: http://blogs.msdn.com/b/vcblog/archive/2010/04/06/c-0x-core-language-features-in-vc10-the-table.aspx I think that there is likely to be an expectation that the same compiler that is used to build pg should be able to include pg headers in C++ TUs. -- Regards, Peter Geoghegan
Tom Lane wrote: > Alvaro Herrera <alvherre@commandprompt.com> writes: > > Excerpts from Tom Lane's message of lun dic 27 13:54:56 -0300 2010: > >> [ lightbulb ] ... although we could improve that quite a bit if we > >> processed each .h file separately instead of insisting on smashing > >> everything into one compilation. Let me go try that. > > > FWIW I have this patch lingering about that I wrote months ago, to check > > for header problems (not C++ stuff, just things like forgetting to > > include some necessary header in some other header). Since it needs a > > lot of polish (needs to ignore certain headers, and avoid leave > > lingering files around), I didn't commit it; and I haven't updated it to > > the new Make recursive stuff, either. > > src/tools/pginclude/ already contains several scripts for this sort of > thing. Bruce runs them by hand occasionally, although I just found out > that he's evidently not run the does-each-header-compile-standalone > test in awhile. It would probably pay to automate these. It is true I have not run those tests in a while. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +