Re: [postgis-tickets] r14592 - Patch from Sebastiaan Couwenberg to fix test_wkb_out_point failure on hppa & mips.

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: [postgis-tickets] r14592 - Patch from Sebastiaan Couwenberg to fix test_wkb_out_point failure on hppa & mips.

Greg Troxel

Regina Obe <[hidden email]> writes:

> Author: robe
> Date: 2016-01-12 16:36:35 -0800 (Tue, 12 Jan 2016)
> New Revision: 14592
>
> Modified:
>    branches/2.2/liblwgeom/cunit/cu_out_wkb.c
> Log:
> Patch from Sebastiaan Couwenberg to fix test_wkb_out_point failure on hppa & mips.
> references #3426

I'm a little surprised at changes going onto the release branch without
first being on trunk and having review and then getting cherrypicked
onto the release branch.

> +/* parisc and mips (at least some processors) have a different nan representation from other arches. */
> +#if !defined(__hppa__) && !defined(__mips__)
> +# define nan_val( v1, v2)  v1
> +#else
> +# define nan_val( v1, v2)  v2
> +#endif

This seems quite awkward.  It would seem the tests should be grounded in
the standards (presumably postgis requires IEEE754 to start and doesn't
work on the vax :-).  Isn't there some way to start with a nan expressed
as a double, and then to convert that to a sql representation?  Or to
write an "isnan_wkb" function that takes the string/hex representation,
extracts a double, and then uses the C99-specified isnan(3) procedures.
Encoding of IEEE754 values into hex and having them in the tests seems
nonrobust, and it seems extra fragile to have switches by arch.

_______________________________________________
postgis-devel mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/postgis-devel

signature.asc (186 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [postgis-tickets] r14592 - Patch from Sebastiaan Couwenberg to fix test_wkb_out_point failure on hppa & mips.

Regina Obe
Greg,

I committed to both trunk and branch 2.2.  I blindly trusted Sebastian's
judgement on things since it was just a regression test (not code change),
he had tested on platforms I have no access to,
and the tests still passed under my mingw64 install.  I'm not knowledgeable
enough in C to judge code.

Perhaps Paul , Sandro, or others can chime in on this.

Thanks,
Regina

-----Original Message-----
From: Greg Troxel [mailto:[hidden email]]
Sent: Tuesday, January 12, 2016 7:44 PM
To: Regina Obe <[hidden email]>
Cc: [hidden email]
Subject: Re: [postgis-tickets] r14592 - Patch from Sebastiaan Couwenberg to
fix test_wkb_out_point failure on hppa & mips.


Regina Obe <[hidden email]> writes:

> Author: robe
> Date: 2016-01-12 16:36:35 -0800 (Tue, 12 Jan 2016) New Revision: 14592
>
> Modified:
>    branches/2.2/liblwgeom/cunit/cu_out_wkb.c
> Log:
> Patch from Sebastiaan Couwenberg to fix test_wkb_out_point failure on hppa
& mips.
> references #3426

I'm a little surprised at changes going onto the release branch without
first being on trunk and having review and then getting cherrypicked onto
the release branch.

> +/* parisc and mips (at least some processors) have a different nan
> +representation from other arches. */ #if !defined(__hppa__) &&
> +!defined(__mips__) # define nan_val( v1, v2)  v1 #else # define
> +nan_val( v1, v2)  v2 #endif

This seems quite awkward.  It would seem the tests should be grounded in the
standards (presumably postgis requires IEEE754 to start and doesn't work on
the vax :-).  Isn't there some way to start with a nan expressed as a
double, and then to convert that to a sql representation?  Or to write an
"isnan_wkb" function that takes the string/hex representation, extracts a
double, and then uses the C99-specified isnan(3) procedures.
Encoding of IEEE754 values into hex and having them in the tests seems
nonrobust, and it seems extra fragile to have switches by arch.


_______________________________________________
postgis-devel mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/postgis-devel
Reply | Threaded
Open this post in threaded view
|

Re: [postgis-tickets] r14592 - Patch from Sebastiaan Couwenberg to fix test_wkb_out_point failure on hppa & mips.

Sandro Santilli-2
On Wed, Jan 13, 2016 at 01:45:07AM -0500, Regina Obe wrote:
> Greg,
>
> I committed to both trunk and branch 2.2.  I blindly trusted Sebastian's
> judgement on things since it was just a regression test (not code change),
> he had tested on platforms I have no access to,
> and the tests still passed under my mingw64 install.  I'm not knowledgeable
> enough in C to judge code.
>
> Perhaps Paul , Sandro, or others can chime in on this.

I've no major problem with the change as it's only in the tester,
but Greg makes a good point about the need to find a portable way
to represent a NaN, if that's what we're using for a POINT EMPY
in WKB form.

WKB supports big-endian or little-endian indication, to be
architecture independent, but if we need to be able to transport
a POINT EMPTY in WKB form from a vax to an x86...

This is probably best discussed in
https://trac.osgeo.org/postgis/ticket/3426

--strk;


>
> Thanks,
> Regina
>
> -----Original Message-----
> From: Greg Troxel [mailto:[hidden email]]
> Sent: Tuesday, January 12, 2016 7:44 PM
> To: Regina Obe <[hidden email]>
> Cc: [hidden email]
> Subject: Re: [postgis-tickets] r14592 - Patch from Sebastiaan Couwenberg to
> fix test_wkb_out_point failure on hppa & mips.
>
>
> Regina Obe <[hidden email]> writes:
>
> > Author: robe
> > Date: 2016-01-12 16:36:35 -0800 (Tue, 12 Jan 2016) New Revision: 14592
> >
> > Modified:
> >    branches/2.2/liblwgeom/cunit/cu_out_wkb.c
> > Log:
> > Patch from Sebastiaan Couwenberg to fix test_wkb_out_point failure on hppa
> & mips.
> > references #3426
>
> I'm a little surprised at changes going onto the release branch without
> first being on trunk and having review and then getting cherrypicked onto
> the release branch.
>
> > +/* parisc and mips (at least some processors) have a different nan
> > +representation from other arches. */ #if !defined(__hppa__) &&
> > +!defined(__mips__) # define nan_val( v1, v2)  v1 #else # define
> > +nan_val( v1, v2)  v2 #endif
>
> This seems quite awkward.  It would seem the tests should be grounded in the
> standards (presumably postgis requires IEEE754 to start and doesn't work on
> the vax :-).  Isn't there some way to start with a nan expressed as a
> double, and then to convert that to a sql representation?  Or to write an
> "isnan_wkb" function that takes the string/hex representation, extracts a
> double, and then uses the C99-specified isnan(3) procedures.
> Encoding of IEEE754 values into hex and having them in the tests seems
> nonrobust, and it seems extra fragile to have switches by arch.
>
>
> _______________________________________________
> postgis-devel mailing list
> [hidden email]
> http://lists.osgeo.org/mailman/listinfo/postgis-devel

--

  ()   Free GIS & Flash consultant/developer
  /\   http://strk.keybit.net/services.html
_______________________________________________
postgis-devel mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/postgis-devel
Reply | Threaded
Open this post in threaded view
|

Re: [postgis-tickets] r14592 - Patch from Sebastiaan Couwenberg to fix test_wkb_out_point failure on hppa & mips.

Paul Ramsey
We already have a platform-dependent generator of NaN-in-hex, and it’s in the WKB output code :) If we start comparing the output of WKB to a generated NaN in the tester, we might as well remove the tests (which I’m fine w/). Or find another Nan generator to use? We use any defined NAN macro, or if there isn’t one, we use

#ifndef NAN
#define NAN 0.0/0.0
#endif

P

On Jan 13, 2016, at 3:24 AM, Sandro Santilli <[hidden email]> wrote:

On Wed, Jan 13, 2016 at 01:45:07AM -0500, Regina Obe wrote:
Greg,

I committed to both trunk and branch 2.2.  I blindly trusted Sebastian's
judgement on things since it was just a regression test (not code change),
he had tested on platforms I have no access to,
and the tests still passed under my mingw64 install.  I'm not knowledgeable
enough in C to judge code.

Perhaps Paul , Sandro, or others can chime in on this.

I've no major problem with the change as it's only in the tester,
but Greg makes a good point about the need to find a portable way
to represent a NaN, if that's what we're using for a POINT EMPY
in WKB form.

WKB supports big-endian or little-endian indication, to be
architecture independent, but if we need to be able to transport
a POINT EMPTY in WKB form from a vax to an x86...

This is probably best discussed in
https://trac.osgeo.org/postgis/ticket/3426

--strk;



Thanks,
Regina

-----Original Message-----
From: Greg Troxel [[hidden email]] 
Sent: Tuesday, January 12, 2016 7:44 PM
To: Regina Obe <[hidden email]>
Cc: [hidden email]
Subject: Re: [postgis-tickets] r14592 - Patch from Sebastiaan Couwenberg to
fix test_wkb_out_point failure on hppa & mips.


Regina Obe <[hidden email]> writes:

Author: robe
Date: 2016-01-12 16:36:35 -0800 (Tue, 12 Jan 2016) New Revision: 14592

Modified:
  branches/2.2/liblwgeom/cunit/cu_out_wkb.c
Log:
Patch from Sebastiaan Couwenberg to fix test_wkb_out_point failure on hppa
& mips.
references #3426

I'm a little surprised at changes going onto the release branch without
first being on trunk and having review and then getting cherrypicked onto
the release branch.

+/* parisc and mips (at least some processors) have a different nan 
+representation from other arches. */ #if !defined(__hppa__) && 
+!defined(__mips__) # define nan_val( v1, v2)  v1 #else # define 
+nan_val( v1, v2)  v2 #endif

This seems quite awkward.  It would seem the tests should be grounded in the
standards (presumably postgis requires IEEE754 to start and doesn't work on
the vax :-).  Isn't there some way to start with a nan expressed as a
double, and then to convert that to a sql representation?  Or to write an
"isnan_wkb" function that takes the string/hex representation, extracts a
double, and then uses the C99-specified isnan(3) procedures.
Encoding of IEEE754 values into hex and having them in the tests seems
nonrobust, and it seems extra fragile to have switches by arch.


_______________________________________________
postgis-devel mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/postgis-devel

-- 

 ()   Free GIS & Flash consultant/developer
 /\   http://strk.keybit.net/services.html
_______________________________________________
postgis-devel mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/postgis-devel


_______________________________________________
postgis-devel mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/postgis-devel