Обсуждение: Patch: make behavior of all versions of the "isinf" function be similar
Hello, hackers! While I was searching for a function which checks doubles for infinity, I discovered a function "isinf" in a file src/port/isinf.c where one of three versions returns different value for "-inf" ("1" instead of "-1") comparing to the other two. It seems concrete values (not just "if isinf(...)") are checked only in float.c in float4out and float8out, but I am going to check for concrete values in my another patch. For systems with HAVE_FPCLASS the function returns the same result for both "+inf" and "-inf". I can't test it on my WS, but I found only one man page[1] where system header file "ieeefp.h" must be included for ability to use "fpclass" function. I guess nothing will be broken if that version of the function returns the same results for input values as the other two. Proposed patch makes that behavior. P.S.: Should the patch be added to the next CF? [1]https://docs.oracle.com/cd/E36784_01/html/E36874/fpclass-3c.html -- Best regards, Vitaly Burovoy
Вложения
Re: Patch: make behavior of all versions of the "isinf" function be similar
От
Michael Paquier
Дата:
On Mon, Feb 1, 2016 at 8:13 AM, Vitaly Burovoy <vitaly.burovoy@gmail.com> wrote: > While I was searching for a function which checks doubles for > infinity, I discovered a function "isinf" in a file src/port/isinf.c > where one of three versions returns different value for "-inf" ("1" > instead of "-1") comparing to the other two. > > For systems with HAVE_FPCLASS the function returns the same result for > both "+inf" and "-inf". I can't test it on my WS, but I found only one > man page[1] where system header file "ieeefp.h" must be included for > ability to use "fpclass" function. That's the case on OSX, but on Linux -1 is returned for -Inf, and 1 for +Inf. > I guess nothing will be broken if that version of the function returns > the same results for input values as the other two. > Proposed patch makes that behavior. Actually, is there actually a reason to keep this file in the code tree? Are there platforms that do not have isinf()? Even for Windows environments using MSVC < 1800 this is emulated using _fpclass. -- Michael
Re: Patch: make behavior of all versions of the "isinf" function be similar
От
Michael Paquier
Дата:
On Mon, Feb 1, 2016 at 2:38 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > Actually, is there actually a reason to keep this file in the code > tree? Are there platforms that do not have isinf()? Even for Windows > environments using MSVC < 1800 this is emulated using _fpclass. Looking at what is in the buildfarm, I noticed that isinf is provided everywhere. Attached is a patch. Thoughts? -- Michael
Вложения
Michael Paquier <michael.paquier@gmail.com> writes: > On Mon, Feb 1, 2016 at 2:38 PM, Michael Paquier > <michael.paquier@gmail.com> wrote: >> Actually, is there actually a reason to keep this file in the code >> tree? Are there platforms that do not have isinf()? Even for Windows >> environments using MSVC < 1800 this is emulated using _fpclass. > Looking at what is in the buildfarm, I noticed that isinf is provided > everywhere. Attached is a patch. Thoughts? Two comments: 1. I don't think the buildfarm is sufficient evidence to conclude that isinf.c is required nowhere. It was in use as late as 2004, judging by the git history, and I don't know of good reason to assume we do not need it now. 2. POSIX:2008 only requires that "The isinf() macro shall return a non-zero value if and only if its argument has an infinite value." Therefore, any assumption about the sign of the result is wrong anyway, and any patch that depends on it will be rejected, regardless of what isinf.c does. Or else, if you can convince us that such an assumption is really valuable, we'd need isinf.c more not less so that we can replace isinf() on platforms where it doesn't meet the stronger spec. regards, tom lane
Re: Patch: make behavior of all versions of the "isinf" function be similar
От
Michael Paquier
Дата:
On Mon, Feb 1, 2016 at 4:04 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
1. I don't think the buildfarm is sufficient evidence to conclude that
isinf.c is required nowhere. It was in use as late as 2004, judging
by the git history, and I don't know of good reason to assume we do not
need it now.
This was 12 years ago... Btw, after a bit of digging, I found out that even SunOS 5.1 includes it:
http://www.unix.com/man-page/sunos/3m/isinf/
-- http://www.unix.com/man-page/sunos/3m/isinf/
Michael
On 1/31/16, Tom Lane <tgl@sss.pgh.pa.us> wrote: > 2. POSIX:2008 only requires that "The isinf() macro shall return a > non-zero value if and only if its argument has an infinite value." > Therefore, any assumption about the sign of the result is wrong anyway, > and any patch that depends on it will be rejected, regardless of what > isinf.c does. Or else, if you can convince us that such an assumption > is really valuable, we'd need isinf.c more not less so that we can > replace isinf() on platforms where it doesn't meet the stronger spec. > > regards, tom lane Ok, then I'll use "is_infinite" from "float.c". But why functions' (in "src/port/isinf.c") behavior are different? It is a bit confusing… -- Best regards, Vitaly Burovoy
Vitaly Burovoy <vitaly.burovoy@gmail.com> writes: > On 1/31/16, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> 2. POSIX:2008 only requires that "The isinf() macro shall return a >> non-zero value if and only if its argument has an infinite value." > Ok, then I'll use "is_infinite" from "float.c". Yeah, that's good. > But why functions' (in "src/port/isinf.c") behavior are different? It > is a bit confusing… Probably the authors of those different implementations were making it match the behavior of whatever isinf() they had locally. I don't feel a need to change isinf.c --- it should be zero-maintenance at this point, especially if we suspect it is no longer used anywhere. regards, tom lane