Thread: [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
Attachment
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 included
>
> Well, 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