Обсуждение: uintptr_t for Datum
Attached patch is the part of the win64 patch that changes Datum to be uintptr_t, and associated changes, with only very minor changes from me. It also includes autoconf tests that I tricked Bruce into fixing for me :-) Comments? Unless there are objections, I'll go ahead and apply this one for broader buildfarm testing. It's working on all the platforms I could test, and also on Bruces (where the original patch broke). -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Вложения
Magnus Hagander <magnus@hagander.net> writes:
> Attached patch is the part of the win64 patch that changes Datum to be
> uintptr_t, and associated changes, with only very minor changes from
> me. It also includes autoconf tests that I tricked Bruce into fixing
> for me :-)
> Comments?
This is a joke no? Where's the logic to provide a definition of
intptr_t if the platform fails to? The lack of attention to updating
the comments about Datum doesn't give me a warm feeling either.
BTW, it looks like the patch is showing a manual change to
pg_config.h.in. Don't do that. Run autoheader.
regards, tom lane
2009/12/31 Tom Lane <tgl@sss.pgh.pa.us>: > Magnus Hagander <magnus@hagander.net> writes: >> Attached patch is the part of the win64 patch that changes Datum to be >> uintptr_t, and associated changes, with only very minor changes from >> me. It also includes autoconf tests that I tricked Bruce into fixing >> for me :-) > >> Comments? > > This is a joke no? Hey, it got your attention ;) > Where's the logic to provide a definition of > intptr_t if the platform fails to? The lack of attention to updating autoconf does that. This is exactly what broke on Bruce's platform, and autoconf fixed it in the way that is included in the patch. > the comments about Datum doesn't give me a warm feeling either. Will look over that. > BTW, it looks like the patch is showing a manual change to > pg_config.h.in. Don't do that. Run autoheader. That also came out of Bruce's patch. Bruce, can you look at doing that? I don't have a machine easily accessible with the right autoconf version ATM :( -- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
Magnus Hagander <magnus@hagander.net> writes:
> 2009/12/31 Tom Lane <tgl@sss.pgh.pa.us>:
>> �Where's the logic to provide a definition of
>> intptr_t if the platform fails to?
> autoconf does that.
Oh, that's what I get for trying to review a patch before absorbing
any caffeine :-( ... I missed that you were relying on a built-in
autoconf macro.
> That also came out of Bruce's patch. Bruce, can you look at doing
> that? I don't have a machine easily accessible with the right autoconf
> version ATM :(
It's a really bad idea to be committing configure changes without having
personally run the patch through autoconf.
As penance for being too quick to complain, I'll review and commit this
myself. If it works on my old HPUX box, it'll probably work everywhere ;-)
regards, tom lane
2009/12/31 Tom Lane <tgl@sss.pgh.pa.us>: > Magnus Hagander <magnus@hagander.net> writes: >> 2009/12/31 Tom Lane <tgl@sss.pgh.pa.us>: >>> Where's the logic to provide a definition of >>> intptr_t if the platform fails to? > >> autoconf does that. > > Oh, that's what I get for trying to review a patch before absorbing > any caffeine :-( ... I missed that you were relying on a built-in > autoconf macro. :-) >> That also came out of Bruce's patch. Bruce, can you look at doing >> that? I don't have a machine easily accessible with the right autoconf >> version ATM :( > > It's a really bad idea to be committing configure changes without having > personally run the patch through autoconf. Right, this is why I had Bruce do that part, and send it to me separately. I figured one committer is as good as another. > As penance for being too quick to complain, I'll review and commit this > myself. If it works on my old HPUX box, it'll probably work everywhere ;-) Ok, deal :-) That's probably the one other platform beside Bruce's that gets reasonably-regular-testing and still doesn't have intptr_t. I'll be off to my newyears party now, enjoy the patch! Happy new year to you and other PostgreSQL hackers! -- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
Tom Lane wrote: > Magnus Hagander <magnus@hagander.net> writes: > > Attached patch is the part of the win64 patch that changes Datum to be > > uintptr_t, and associated changes, with only very minor changes from > > me. It also includes autoconf tests that I tricked Bruce into fixing > > for me :-) > > > Comments? > > This is a joke no? Where's the logic to provide a definition of > intptr_t if the platform fails to? The lack of attention to updating > the comments about Datum doesn't give me a warm feeling either. > > BTW, it looks like the patch is showing a manual change to > pg_config.h.in. Don't do that. Run autoheader. I wasn't aware autoheader existed. Is that new or has it alwasy been part of autoconf? Attached is the diff for pg_config.h.in generated by autoheader. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + Index: pg_config.h.in =================================================================== RCS file: /cvsroot/pgsql/src/include/pg_config.h.in,v retrieving revision 1.143 diff -c -r1.143 pg_config.h.in *** pg_config.h.in 1 Oct 2009 01:58:58 -0000 1.143 --- pg_config.h.in 31 Dec 2009 17:35:33 -0000 *************** *** 236,241 **** --- 236,244 ---- /* Define to 1 if the system has the type `int8'. */ #undef HAVE_INT8 + /* Define to 1 if the system has the type `intptr_t'. */ + #undef HAVE_INTPTR_T + /* Define to 1 if you have the <inttypes.h> header file. */ #undef HAVE_INTTYPES_H *************** *** 599,604 **** --- 602,610 ---- /* Define to 1 if the system has the type `uint8'. */ #undef HAVE_UINT8 + /* Define to 1 if the system has the type `uintptr_t'. */ + #undef HAVE_UINTPTR_T + /* Define to 1 if the system has the type `union semun'. */ #undef HAVE_UNION_SEMUN *************** *** 827,835 **** --- 833,849 ---- #undef inline #endif + /* Define to the type of a signed integer type wide enough to hold a pointer, + if such a type exists, and if the system does not define it. */ + #undef intptr_t + /* Define to empty if the C compiler does not understand signed types. */ #undef signed + /* Define to the type of an unsigned integer type wide enough to hold a + pointer, if such a type exists, and if the system does not define it. */ + #undef uintptr_t + /* Define to empty if the keyword `volatile' does not work. Warning: valid code using `volatile' can become incorrect without. Disable with care. */ #undef volatile
Bruce Momjian <bruce@momjian.us> writes:
> Tom Lane wrote:
>> BTW, it looks like the patch is showing a manual change to
>> pg_config.h.in. Don't do that. Run autoheader.
> I wasn't aware autoheader existed. Is that new or has it alwasy been
> part of autoconf?
It's always been there, or at least for many years. pg_config.h.in
really ought to be thought of the same as configure: you don't edit
it, you just generate it.
regards, tom lane
Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > Tom Lane wrote: > >> BTW, it looks like the patch is showing a manual change to > >> pg_config.h.in. Don't do that. Run autoheader. > > > I wasn't aware autoheader existed. Is that new or has it alwasy been > > part of autoconf? > > It's always been there, or at least for many years. pg_config.h.in > really ought to be thought of the same as configure: you don't edit > it, you just generate it. Well, that's pretty confusing considering it has a .in suffix, just like configure.in, which we do edit, but I get your point. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
On tor, 2009-12-31 at 13:44 -0500, Bruce Momjian wrote: > Tom Lane wrote: > > Bruce Momjian <bruce@momjian.us> writes: > > > Tom Lane wrote: > > >> BTW, it looks like the patch is showing a manual change to > > >> pg_config.h.in. Don't do that. Run autoheader. > > > > > I wasn't aware autoheader existed. Is that new or has it alwasy been > > > part of autoconf? > > > > It's always been there, or at least for many years. pg_config.h.in > > really ought to be thought of the same as configure: you don't edit > > it, you just generate it. > > Well, that's pretty confusing considering it has a .in suffix, just like > configure.in, which we do edit, but I get your point. I realize it's easy to miss, but the first line of pg_config.h.in tells that it is generated.