Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make parson -Wfloat-equal compatible #104

Open
msteinbeck opened this issue Aug 2, 2018 · 8 comments
Open

Make parson -Wfloat-equal compatible #104

msteinbeck opened this issue Aug 2, 2018 · 8 comments

Comments

@msteinbeck
Copy link

I try to include parson into a project which compiles the source files with -Wfloat-equal. Unfortunately, parson compares double values: https://github.com/kgabis/parson/blob/master/parson.c#L1367. As far as I can see this is the only line.

@kgabis
Copy link
Owner

kgabis commented Aug 2, 2018

Should be fixed now (if your standard library defines isnan and isinf).

@kgabis kgabis closed this as completed Aug 2, 2018
@msteinbeck
Copy link
Author

You patch, unfortunately, does not work. GCC and Clang on Linux, at least on my system, do not provide isnan and isinf. How about the following snippet:

#ifdef __GNUC__
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wfloat-equal"
#endif
    if ((number * 0.0) != 0.0) { /* nan and inf test */
#ifdef __GNUC__
#pragma GCC diagnostic pop
#endif

It temporarily disables the warning and works with GCC and Clang.

@kgabis
Copy link
Owner

kgabis commented Aug 2, 2018

What compiler (version) on what sytem are you using and what flags do you use to compile parson?

@kgabis kgabis reopened this Aug 2, 2018
@msteinbeck
Copy link
Author

msteinbeck commented Aug 3, 2018

I'm running Slackware 14.2 with gcc-5.5.0, clang-3.8.0, and glibc-2.23. I attached a small example, containing my library (tinyspline), parson (the latest commit), and an example which prints the resulting JSON String to stdout. I use the following command to compile the program:

gcc -std=c89 -Werror -Wall -Wextra -Wfloat-equal -pedantic -o json_export json_export.c parson.c tinyspline.c -lm

I get the following error:

parson.c: In function ‘json_value_init_number’:
parson.c:59:43: error: comparing floating point with == or != is unsafe [-Werror=float-equal]
 #define IS_NUMBER_INVALID(x) (((x) * 0.0) != 0.0)
                                           ^
parson.c:1373:9: note: in expansion of macro ‘IS_NUMBER_INVALID’
     if (IS_NUMBER_INVALID(number)) {
         ^
cc1: all warnings being treated as errors

Contrary to my previous tests, Clang compiles without any issues:

clang -std=c89 -Werror -Wall -Wextra -Wfloat-equal -pedantic -o json_export json_export.c parson.c tinyspline.c -lm

Attachment: parson.zip

EDIT: Update zip file.

@kgabis
Copy link
Owner

kgabis commented Aug 3, 2018

I’m unable to download your attachment, could you mail it to me?

@msteinbeck
Copy link
Author

You should now be able to download the attachment. Maybe uMatrix blocked Github's upload service. Anyhow, I mailed you the attachment.

@kgabis
Copy link
Owner

kgabis commented Aug 4, 2018

As far as I understand some versions of libc don't provide isnan unless you compile your code using c99 standard or newer (#define isnan is guarded by #ifdef __USE_ISOC99). On my mac there is no such guard, but when I compiled it on gnu/linux the problem appeared. clang on the other hand doesn't generate a warning when comparing to a float constant that's fully representable.
Example:

int comp(double x, double y)
{
        int test1 = x == 0.0;
        int test2 = x == 0.1; /* warning */
        int test3 = x == 1.0;
        int test4 = x == 2.0;
        int test5 = x == 3.0;
        int test6 = x == 4.0;
        int test7 = x == 4.4; /* warning */
        int test8 = x == y; /* warning */
        return test1 || test2 || test3 || test4 || test5 || test6 || test7 || test8;
}

It's a bit worrying because no warning is generated for this function:

int comp()
{
        double acc = 0.0;
        for (int x = 0; x < 10; x++) {
                acc += 0.1;
        }
        return acc == 1.0; /* no warning in clang */
}

Anyway, I'd rather not add #pragma GCC diagnostic ignored "-Wfloat-equal" because it's feels like a hack and it won't scale if other people start reporting such problems. Is there a way for you to disable this warning when compiling parson.c (such as -w flag in gcc)?

@msteinbeck
Copy link
Author

Thank you for your work.

Is there a way for you to disable this warning when compiling parson.c (such as -w flag in gcc)?

I don't want to compile external libraries (currently it's only parson) separately and, thus, use the same compiler flags for all source files. This decision is the basis of the fact that I want to keep the option open that others can simply include tinyspline's source files into their projects without depending on tinyspline's build system (CMake)---similar to parson. That's why I compile tinyspline with -Werror -Wall -Wextra -Wfloat-equal -pedantic for GCC and Clang, and /Wall /WX for MSVC. Warnings that can't be fixed are disabled temporarily. It ensures that the source files are as portable as possible. Otherwise, projects using tinyspline may be in the same situation as I am with parson; including the source files requires some extra effort.

Anyhow, I can still maintain a modified version of parson in tinyspline's repository. The required modifications aren't too invasive.

Finally, I would like to thank you for this great library. You did a great job!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants