[PATCH] Move 'long long' check to c.h
Greetings,
While reviewing bfba40e2c7b3909d3de13bd1b83b7e85fa8dfec2 (mmm, we like
git diff -p), I noted that c.h is already included by both extern.h
and ecpg.header through postgres_fe.h. Given this and that we're
already doing alot of similar #define's there (unlike in those other
files), I felt c.h was a more appropriate place. Putting it in c.h
also means we don't have to duplicate that code.
Patch attached.
Thanks,
Stephen
Attachments:
pg-long-long.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/include/c.h b/src/include/c.h
index ea31635..d729e04 100644
*** a/src/include/c.h
--- b/src/include/c.h
*************** typedef unsigned long long int uint64;
*** 284,289 ****
--- 284,294 ----
#error must have a working 64-bit integer datatype
#endif
+ /* Do we know the C99 data type "long long"? */
+ #if defined(LLONG_MIN) || defined(LONGLONG_MIN) || defined(HAVE_LONG_LONG_INT_64)
+ #define HAVE_LONG_LONG 1
+ #endif
+
/* Decide if we need to decorate 64-bit constants */
#ifdef HAVE_LL_CONSTANTS
#define INT64CONST(x) ((int64) x##LL)
diff --git a/src/interfaces/ecpg/ecpglib/extern.h b/src/interfaces/ecpg/ecpglib/extern.h
index e3a7c82..7a5259f 100644
*** a/src/interfaces/ecpg/ecpglib/extern.h
--- b/src/interfaces/ecpg/ecpglib/extern.h
***************
*** 13,23 ****
#include <limits.h>
#endif
- /* Do we know the C99 data type "long long"? */
- #if defined(LLONG_MIN) || defined(LONGLONG_MIN) || defined(HAVE_LONG_LONG_INT_64)
- #define HAVE_LONG_LONG 1
- #endif
-
enum COMPAT_MODE
{
ECPG_COMPAT_PGSQL = 0, ECPG_COMPAT_INFORMIX, ECPG_COMPAT_INFORMIX_SE
--- 13,18 ----
diff --git a/src/interfaces/ecpg/preproc/ecpg.header b/src/interfaces/ecpg/preproc/ecpg.header
index dcc87a1..6b05776 100644
*** a/src/interfaces/ecpg/preproc/ecpg.header
--- b/src/interfaces/ecpg/preproc/ecpg.header
***************
*** 7,17 ****
#include "extern.h"
#include <unistd.h>
- /* Do we know the C99 datatype "long long"? */
- #if defined(LLONG_MIN) || defined(LONGLONG_MIN) || defined(HAVE_LONG_LONG_INT_64)
- #define HAVE_LONG_LONG 1
- #endif
-
/* Location tracking support --- simpler than bison's default */
#define YYLLOC_DEFAULT(Current, Rhs, N) \
do { \
--- 7,12 ----
Stephen Frost <sfrost@snowman.net> writes:
While reviewing bfba40e2c7b3909d3de13bd1b83b7e85fa8dfec2 (mmm, we like
git diff -p), I noted that c.h is already included by both extern.h
and ecpg.header through postgres_fe.h. Given this and that we're
already doing alot of similar #define's there (unlike in those other
files), I felt c.h was a more appropriate place. Putting it in c.h
also means we don't have to duplicate that code.
Ugh. Moving that to c.h doesn't render it not junk code. (For one
thing, it will not operate as intended if you haven't previously
#included <limits.h>, which in fact is not included in c.h.)
If we need this we should do it properly with autoconf.
regards, tom lane
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
Ugh. Moving that to c.h doesn't render it not junk code. (For one
thing, it will not operate as intended if you haven't previously
#included <limits.h>, which in fact is not included in c.h.)
Doh.
If we need this we should do it properly with autoconf.
My autoconf foo is not very good, but once I finish a couple of other
things I'll take a shot at doing it that way.
Thanks!
Stephen
On Sat, May 22, 2010 at 11:20:50PM -0400, Stephen Frost wrote:
git diff -p), I noted that c.h is already included by both extern.h
and ecpg.header through postgres_fe.h. Given this and that we're
already doing alot of similar #define's there (unlike in those other
files), I felt c.h was a more appropriate place. Putting it in c.h
also means we don't have to duplicate that code.
But do other parts of PG also need it? Keep in mind that this works for ecpg
because it needs LLONG_MIN or LONGLONG_MIN anyway. I'm not sure if there are
compilers that have long long without those defines, but I'd guess there
aren't.
Michael
--
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org
ICQ 179140304, AIM/Yahoo/Skype michaelmeskes, Jabber meskes@jabber.org
VfL Borussia! Força Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL
On Sun, May 23, 2010 at 11:50:00AM -0400, Stephen Frost wrote:
If we need this we should do it properly with autoconf.
I absolutely agree and planed to do that *after* the release if it makes sense
for the rest of PG, but wouldn't want to mess with it in the current
situtation. On the other hand I didn't want to release with that bug in there.
Michael
--
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org
ICQ 179140304, AIM/Yahoo/Skype michaelmeskes, Jabber meskes@jabber.org
VfL Borussia! Força Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL
Michael Meskes <meskes@postgresql.org> writes:
On Sat, May 22, 2010 at 11:20:50PM -0400, Stephen Frost wrote:
git diff -p), I noted that c.h is already included by both extern.h
and ecpg.header through postgres_fe.h. Given this and that we're
already doing alot of similar #define's there (unlike in those other
files), I felt c.h was a more appropriate place. Putting it in c.h
also means we don't have to duplicate that code.
But do other parts of PG also need it? Keep in mind that this works for ecpg
because it needs LLONG_MIN or LONGLONG_MIN anyway. I'm not sure if there are
compilers that have long long without those defines, but I'd guess there
aren't.
I think the current coding is extremely fragile (if it indeed works at
all) because of its assumption that <limits.h> has been included
already. In any case, we have configure tests that exist only for the
benefit of contrib modules, so it's hard to argue that we shouldn't have
one that exists only for ecpg.
I think we should fix this (properly) for 9.0.
regards, tom lane
I think the current coding is extremely fragile (if it indeed works at
all) because of its assumption that <limits.h> has been included
Well, this is the case in the code so far.
already. In any case, we have configure tests that exist only for the
benefit of contrib modules, so it's hard to argue that we shouldn't have
one that exists only for ecpg.I think we should fix this (properly) for 9.0.
Ok, I don't mind fixing it properly for 9.0. Will do so as soon as I find time.
Michael
--
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org
ICQ 179140304, AIM/Yahoo/Skype michaelmeskes, Jabber meskes@jabber.org
VfL Borussia! Força Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL
* Michael Meskes (meskes@postgresql.org) wrote:
I think the current coding is extremely fragile (if it indeed works at
all) because of its assumption that <limits.h> has been includedWell, this is the case in the code so far.
Right, the existing code is after limits.h is included, my suggestion to
put it in c.h would have lost limits.h and broken things. Sorry about
that. I didn't realize the dependency and make check didn't complain
(not that I'm sure there's even a way we could have a regression test
for this..). I didn't intend to imply the currently-committed code
didn't work (I figured it was probably fine :), was just trying to tidy
a bit.
Thanks!
Stephen