Horton

Thread

Commits

  1. Silence MSVC warnings about redefinition of isnan

  2. Rethink how to get float.h in old Windows API for isnan/isinf

  1. _isnan() on Windows

    Emre Hasegeli <emre@hasegeli.com> — 2018-07-10T17:36:39Z

    isnan() function is evidently not present on <math.h> on Windows
before Visual Studio 2013.  We define it on win32_port.h using
_isnan().  However _isnan() is also not present.  It is on <float.h>.
The patch is attached to include this from win32_port.h.

Thanks to Thomas Munro for point this out to me [1].  It is hard to
notice this issue without testing the changes on Windows.

[1] https://www.postgresql.org/message-id/CAEepm%3D3dE0w%3D_dOcELpPum6suze2NZwc%3DAV4T_xYrDUoHbZJvA%40mail.gmail.com

  2. Re: _isnan() on Windows

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-07-10T17:51:29Z

    On 2018-Jul-10, Emre Hasegeli wrote:

> isnan() function is evidently not present on <math.h> on Windows
> before Visual Studio 2013.  We define it on win32_port.h using
> _isnan().  However _isnan() is also not present.  It is on <float.h>.
> The patch is attached to include this from win32_port.h.
> 
> Thanks to Thomas Munro for point this out to me [1].  It is hard to
> notice this issue without testing the changes on Windows.

Oh, it looks like commits 33a7101281c6, 8e211f539146, 86dbbf20d849
(probably others) papered over this by the expedient of adding #include
<float.h> to random .c files rather than your patch, which seems the
proper fix.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  3. Re: _isnan() on Windows

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-07-10T19:55:13Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2018-Jul-10, Emre Hasegeli wrote:
>> isnan() function is evidently not present on <math.h> on Windows
>> before Visual Studio 2013.  We define it on win32_port.h using
>> _isnan().  However _isnan() is also not present.  It is on <float.h>.
>> The patch is attached to include this from win32_port.h.
>> 
>> Thanks to Thomas Munro for point this out to me [1].  It is hard to
>> notice this issue without testing the changes on Windows.

> Oh, it looks like commits 33a7101281c6, 8e211f539146, 86dbbf20d849
> (probably others) papered over this by the expedient of adding #include
> <float.h> to random .c files rather than your patch, which seems the
> proper fix.

I disagree --- including <float.h> in c.h, as this would have us do,
seems like a huge expansion of the visibility of that header.  Moreover,
doing that only on Windows seems certain to lead to missing-header
problems in the reverse direction, ie patches developed on Windows
will fail elsewhere.

I think we should stick with the existing coding convention of including
that only in the specific .c files that need it.

			regards, tom lane

  4. Re: _isnan() on Windows

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-07-10T20:07:03Z

    On 2018-Jul-10, Tom Lane wrote:

> Alvaro Herrera <alvherre@2ndquadrant.com> writes:

> > Oh, it looks like commits 33a7101281c6, 8e211f539146, 86dbbf20d849
> > (probably others) papered over this by the expedient of adding #include
> > <float.h> to random .c files rather than your patch, which seems the
> > proper fix.
> 
> I disagree --- including <float.h> in c.h, as this would have us do,
> seems like a huge expansion of the visibility of that header.  Moreover,
> doing that only on Windows seems certain to lead to missing-header
> problems in the reverse direction, ie patches developed on Windows
> will fail elsewhere.

I don't think so, because that's only done for MSVC older than 2013.
Nobody uses that for new development anymore.  Downloading versions
prior to 2017 is difficult enough already.

> I think we should stick with the existing coding convention of including
> that only in the specific .c files that need it.

It seems saner to contain the ugliness to a port-specific file, where it
won't pollute two dozen files for no reason.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  5. Re: _isnan() on Windows

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-07-10T20:23:42Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2018-Jul-10, Tom Lane wrote:
>> I disagree --- including <float.h> in c.h, as this would have us do,
>> seems like a huge expansion of the visibility of that header.  Moreover,
>> doing that only on Windows seems certain to lead to missing-header
>> problems in the reverse direction, ie patches developed on Windows
>> will fail elsewhere.

> I don't think so, because that's only done for MSVC older than 2013.
> Nobody uses that for new development anymore.

Hm.  OK, maybe it's all right given that.  I'm still a bit worried
about downsides, but no doubt the buildfarm will tell us.

			regards, tom lane

  6. Re: _isnan() on Windows

    Michael Paquier <michael@paquier.xyz> — 2018-07-11T00:37:54Z

    On Tue, Jul 10, 2018 at 04:23:42PM -0400, Tom Lane wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
>> On 2018-Jul-10, Tom Lane wrote:
>>> I disagree --- including <float.h> in c.h, as this would have us do,
>>> seems like a huge expansion of the visibility of that header.  Moreover,
>>> doing that only on Windows seems certain to lead to missing-header
>>> problems in the reverse direction, ie patches developed on Windows
>>> will fail elsewhere.
> 
>> I don't think so, because that's only done for MSVC older than 2013.
>> Nobody uses that for new development anymore.
> 
> Hm.  OK, maybe it's all right given that.  I'm still a bit worried
> about downsides, but no doubt the buildfarm will tell us.

It seems to me that this patch is a good idea.  Any objections if I take
care of it?  I have a Windows VM with only MSVC 2015 if I recall
correctly though...
--
Michael

  7. Re: _isnan() on Windows

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-07-11T13:13:40Z

    On 2018-Jul-11, Michael Paquier wrote:

> On Tue, Jul 10, 2018 at 04:23:42PM -0400, Tom Lane wrote:
> > Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> >> On 2018-Jul-10, Tom Lane wrote:
> >>> I disagree --- including <float.h> in c.h, as this would have us do,
> >>> seems like a huge expansion of the visibility of that header.  Moreover,
> >>> doing that only on Windows seems certain to lead to missing-header
> >>> problems in the reverse direction, ie patches developed on Windows
> >>> will fail elsewhere.
> > 
> >> I don't think so, because that's only done for MSVC older than 2013.
> >> Nobody uses that for new development anymore.
> > 
> > Hm.  OK, maybe it's all right given that.  I'm still a bit worried
> > about downsides, but no doubt the buildfarm will tell us.
> 
> It seems to me that this patch is a good idea.  Any objections if I take
> care of it?  I have a Windows VM with only MSVC 2015 if I recall
> correctly though...

I just pushed it before seeing your message.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  8. Re: _isnan() on Windows

    Michael Paquier <michael@paquier.xyz> — 2018-07-12T02:16:25Z

    On Wed, Jul 11, 2018 at 09:13:40AM -0400, Alvaro Herrera wrote:
> I just pushed it before seeing your message.

Fine as well, thanks for picking this up.  The buildfarm shows no
failures about this patch.
--
Michael

  9. Re: _isnan() on Windows

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-07-12T14:20:22Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Wed, Jul 11, 2018 at 09:13:40AM -0400, Alvaro Herrera wrote:
>> I just pushed it before seeing your message.

> Fine as well, thanks for picking this up.  The buildfarm shows no
> failures about this patch.

I scraped all the compiler warnings from the buildfarm this morning,
and I see no new ones that could be blamed on this change, so I think
we're good.

bowerbird and hamerkop have some gripes like this:

 bowerbird     | c:\perl64\lib\core\win32.h(218): warning C4005: 'isnan' : macro redefinition (src/pl/plperl/SPI.c) [G:\prog\bf\root\HEAD\pgsql.build\plperl.vcxproj]
 bowerbird     | c:\perl64\lib\core\win32.h(218): warning C4005: 'isnan' : macro redefinition (src/pl/plperl/Util.c) [G:\prog\bf\root\HEAD\pgsql.build\plperl.vcxproj]
 bowerbird     | c:\perl64\lib\core\win32.h(218): warning C4005: 'isnan' : macro redefinition (src/pl/plperl/plperl.c) [G:\prog\bf\root\HEAD\pgsql.build\plperl.vcxproj]
 bowerbird     | c:\perl64\lib\core\win32.h(218): warning C4005: 'isnan' : macro redefinition [G:\prog\bf\root\HEAD\pgsql.build\hstore_plperl.vcxproj]
 bowerbird     | c:\perl64\lib\core\win32.h(218): warning C4005: 'isnan' : macro redefinition [G:\prog\bf\root\HEAD\pgsql.build\jsonb_plperl.vcxproj]
 bowerbird     |   c:\perl64\lib\core\win32.h(218): warning C4005: 'isnan' : macro redefinition (src/pl/plperl/SPI.c) [G:\prog\bf\root\HEAD\pgsql.build\plperl.vcxproj]
 bowerbird     |   c:\perl64\lib\core\win32.h(218): warning C4005: 'isnan' : macro redefinition (src/pl/plperl/Util.c) [G:\prog\bf\root\HEAD\pgsql.build\plperl.vcxproj]
 bowerbird     |   c:\perl64\lib\core\win32.h(218): warning C4005: 'isnan' : macro redefinition (src/pl/plperl/plperl.c) [G:\prog\bf\root\HEAD\pgsql.build\plperl.vcxproj]
 bowerbird     |   c:\perl64\lib\core\win32.h(218): warning C4005: 'isnan' : macro redefinition [G:\prog\bf\root\HEAD\pgsql.build\hstore_plperl.vcxproj]
 bowerbird     |   c:\perl64\lib\core\win32.h(218): warning C4005: 'isnan' : macro redefinition [G:\prog\bf\root\HEAD\pgsql.build\jsonb_plperl.vcxproj]

but those were there before too.  Not sure if there's anything
we can/should try to do about that.

			regards, tom lane

  10. Re: _isnan() on Windows

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2018-07-12T14:33:53Z

    
On 07/12/2018 10:20 AM, Tom Lane wrote:
> Michael Paquier <michael@paquier.xyz> writes:
>> On Wed, Jul 11, 2018 at 09:13:40AM -0400, Alvaro Herrera wrote:
>>> I just pushed it before seeing your message.
>> Fine as well, thanks for picking this up.  The buildfarm shows no
>> failures about this patch.
> I scraped all the compiler warnings from the buildfarm this morning,
> and I see no new ones that could be blamed on this change, so I think
> we're good.
>
> bowerbird and hamerkop have some gripes like this:
>
>   bowerbird     | c:\perl64\lib\core\win32.h(218): warning C4005: 'isnan' : macro redefinition (src/pl/plperl/SPI.c) [G:\prog\bf\root\HEAD\pgsql.build\plperl.vcxproj]
>   bowerbird     | c:\perl64\lib\core\win32.h(218): warning C4005: 'isnan' : macro redefinition (src/pl/plperl/Util.c) [G:\prog\bf\root\HEAD\pgsql.build\plperl.vcxproj]
>   bowerbird     | c:\perl64\lib\core\win32.h(218): warning C4005: 'isnan' : macro redefinition (src/pl/plperl/plperl.c) [G:\prog\bf\root\HEAD\pgsql.build\plperl.vcxproj]
>   bowerbird     | c:\perl64\lib\core\win32.h(218): warning C4005: 'isnan' : macro redefinition [G:\prog\bf\root\HEAD\pgsql.build\hstore_plperl.vcxproj]
>   bowerbird     | c:\perl64\lib\core\win32.h(218): warning C4005: 'isnan' : macro redefinition [G:\prog\bf\root\HEAD\pgsql.build\jsonb_plperl.vcxproj]
>   bowerbird     |   c:\perl64\lib\core\win32.h(218): warning C4005: 'isnan' : macro redefinition (src/pl/plperl/SPI.c) [G:\prog\bf\root\HEAD\pgsql.build\plperl.vcxproj]
>   bowerbird     |   c:\perl64\lib\core\win32.h(218): warning C4005: 'isnan' : macro redefinition (src/pl/plperl/Util.c) [G:\prog\bf\root\HEAD\pgsql.build\plperl.vcxproj]
>   bowerbird     |   c:\perl64\lib\core\win32.h(218): warning C4005: 'isnan' : macro redefinition (src/pl/plperl/plperl.c) [G:\prog\bf\root\HEAD\pgsql.build\plperl.vcxproj]
>   bowerbird     |   c:\perl64\lib\core\win32.h(218): warning C4005: 'isnan' : macro redefinition [G:\prog\bf\root\HEAD\pgsql.build\hstore_plperl.vcxproj]
>   bowerbird     |   c:\perl64\lib\core\win32.h(218): warning C4005: 'isnan' : macro redefinition [G:\prog\bf\root\HEAD\pgsql.build\jsonb_plperl.vcxproj]
>
> but those were there before too.  Not sure if there's anything
> we can/should try to do about that.
>
> 			


We actually undef a bunch of things in plperl.h to keep the compiler 
quiet. Maybe we need to add this to the list?

cheers

andrew

-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  11. Re: _isnan() on Windows

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-07-12T14:38:15Z

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
> On 07/12/2018 10:20 AM, Tom Lane wrote:
>> bowerbird and hamerkop have some gripes like this:
>> 
>> bowerbird     | c:\perl64\lib\core\win32.h(218): warning C4005: 'isnan' : macro redefinition (src/pl/plperl/SPI.c) [G:\prog\bf\root\HEAD\pgsql.build\plperl.vcxproj]

> We actually undef a bunch of things in plperl.h to keep the compiler 
> quiet. Maybe we need to add this to the list?

Perhaps.  But how do we tell the platforms where we should do that
from the ones where we shouldn't?

			regards, tom lane

  12. Re: _isnan() on Windows

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2018-07-12T15:28:46Z

    
On 07/12/2018 10:38 AM, Tom Lane wrote:
> Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
>> On 07/12/2018 10:20 AM, Tom Lane wrote:
>>> bowerbird and hamerkop have some gripes like this:
>>>
>>> bowerbird     | c:\perl64\lib\core\win32.h(218): warning C4005: 'isnan' : macro redefinition (src/pl/plperl/SPI.c) [G:\prog\bf\root\HEAD\pgsql.build\plperl.vcxproj]
>> We actually undef a bunch of things in plperl.h to keep the compiler
>> quiet. Maybe we need to add this to the list?
> Perhaps.  But how do we tell the platforms where we should do that
> from the ones where we shouldn't?
>
> 			

In the _MSCVER section:

#ifdef isnan
#undef isnan
#endif

By inspection the perl header is just defining it to _isnan, for every 
MSC version.

cheers

andrew

-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  13. Re: _isnan() on Windows

    Andres Freund <andres@anarazel.de> — 2018-11-17T19:46:06Z

    Hi,

On 2018-07-12 11:28:46 -0400, Andrew Dunstan wrote:
> On 07/12/2018 10:38 AM, Tom Lane wrote:
> > Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
> > > On 07/12/2018 10:20 AM, Tom Lane wrote:
> > > > bowerbird and hamerkop have some gripes like this:
> > > > 
> > > > bowerbird     | c:\perl64\lib\core\win32.h(218): warning C4005: 'isnan' : macro redefinition (src/pl/plperl/SPI.c) [G:\prog\bf\root\HEAD\pgsql.build\plperl.vcxproj]
> > > We actually undef a bunch of things in plperl.h to keep the compiler
> > > quiet. Maybe we need to add this to the list?
> > Perhaps.  But how do we tell the platforms where we should do that
> > from the ones where we shouldn't?
> > 
> > 			
> 
> In the _MSCVER section:
> 
> #ifdef isnan
> #undef isnan
> #endif
> 
> By inspection the perl header is just defining it to _isnan, for every MSC
> version.

Let's try undefining it before plperl.h then?  It'd be nice to get rid
of those warnings...

Greetings,

Andres Freund

  14. Re: _isnan() on Windows

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2018-11-18T17:46:14Z

    On 11/17/18 2:46 PM, Andres Freund wrote:
> Hi,
>
> On 2018-07-12 11:28:46 -0400, Andrew Dunstan wrote:
>> On 07/12/2018 10:38 AM, Tom Lane wrote:
>>> Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
>>>> On 07/12/2018 10:20 AM, Tom Lane wrote:
>>>>> bowerbird and hamerkop have some gripes like this:
>>>>>
>>>>> bowerbird     | c:\perl64\lib\core\win32.h(218): warning C4005: 'isnan' : macro redefinition (src/pl/plperl/SPI.c) [G:\prog\bf\root\HEAD\pgsql.build\plperl.vcxproj]
>>>> We actually undef a bunch of things in plperl.h to keep the compiler
>>>> quiet. Maybe we need to add this to the list?
>>> Perhaps.  But how do we tell the platforms where we should do that
>>> from the ones where we shouldn't?
>>>
>>> 			
>> In the _MSCVER section:
>>
>> #ifdef isnan
>> #undef isnan
>> #endif
>>
>> By inspection the perl header is just defining it to _isnan, for every MSC
>> version.
> Let's try undefining it before plperl.h then?  It'd be nice to get rid
> of those warnings...
>


done.


cheers


andrew

-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  15. Re: _isnan() on Windows

    Andres Freund <andres@anarazel.de> — 2018-11-18T20:38:01Z

    Hi,

On 2018-11-18 12:46:14 -0500, Andrew Dunstan wrote:
> On 11/17/18 2:46 PM, Andres Freund wrote:
> > On 2018-07-12 11:28:46 -0400, Andrew Dunstan wrote:
> > > By inspection the perl header is just defining it to _isnan, for every MSC
> > > version.
> > Let's try undefining it before plperl.h then?  It'd be nice to get rid
> > of those warnings...

> done.

Thanks!

Greetings,

Andres Freund