Re: 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(at)hagander(dot)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(at)sss(dot)pgh(dot)pa(dot)us>:
> Magnus Hagander <magnus(at)hagander(dot)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 Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> 2009/12/31 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)sss(dot)pgh(dot)pa(dot)us>:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> 2009/12/31 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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 Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

Tom Lane wrote:
> Magnus Hagander <magnus(at)hagander(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)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.