Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#14895 closed enhancement (fixed)

Make wxIsSameDouble compile without warnings with gcc -Wfloat-equal

Reported by: MortenMacFly Owned by:
Priority: low Milestone:
Component: base Version: stable-latest
Keywords: warning float comparison simple Cc:
Blocked By: Blocking:
Patch: no

Description

When working with wxWidgets projects, GCC (4.7.1, Windows32) and using the -Wfloat-equal warnings witch, you get tons of errors like this:

wxWidgets/include/wx/math.h:102:70: warning: comparing floating point with == or != is unsafe [-Wfloat-equal]

...that spam the build log.
This applies to:
inline bool wxIsSameDouble(double x, double y)

Why not doing it properly, for example like this:

{

std::numeric_limits<double> double_info; requires #include <limits>
return (fabs(x-y) < (double_info.epsilon() * 2.0));

}

(or alternatively use just DBL_EPSILON from <cfloat>)???

Attachments (1)

float-equal.patch download (1.9 KB) - added by jens 2 years ago.
silent warning about comparison of doubles with equality operator in wxIsSameDouble

Download all attachments as: .zip

Change History (17)

comment:1 follow-up: Changed 2 years ago by vadz

  • Priority changed from normal to low
  • Summary changed from wxIsSameDouble spams warnings a lot to Make wxIsSameDouble compile without warnings with gcc -Wfloat-equal

There is no way to write a function comparing 2 doubles "correctly" in all cases e.g. both methods you propose above fail completely for small numbers. This function is meant to be used just to avoid the warnings so ideal would be to find some way to suppress them for gcc but I don't know how to do it.

comment:2 Changed 2 years ago by MortenMacFly

Well for small number,s you could use double_info.min() instead. This would work for small number, too.

comment:3 in reply to: ↑ 1 Changed 2 years ago by jens

Replying to vadz:

This function is meant to be used just to avoid the warnings so ideal would be to find some way to suppress them for gcc but I don't know how to do it.

This should suppress the warnings:

inline bool wxIsSameDouble(double x, double y) {
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wfloat-equal"
    return x == y;
#pragma GCC diagnostic pop
}

It should probably get some #ifdefs around to not conflict with other compilers.

Jens

comment:4 Changed 2 years ago by vadz

  • Keywords simple added
  • Status changed from new to confirmed
  • Version changed from 2.8.12 to 2.9-svn

Thanks, this is indeed the right solution.

According to http://gcc.gnu.org/ml/gcc-help/2011-01/msg00112.html these pragmas are all available only starting from 4.6 so should be surrounded with wxCHECK_GCC_VERSION(4,6). Could someone please test whether this works correctly and make a patch with the changes if it does?

comment:5 Changed 2 years ago by jens

The attached patch silents the warning in math.h.

But there are still tons of warnings due to the equality operator in wxAny.

The question is, whether this should be silenced there or wxIsSameDouble should be used.
Probably with an implicit (or explicit) cast to double for the float== of wxAny.

Jens

comment:6 Changed 2 years ago by vadz

The case of wxAny is more complicated because it could indeed be an error to compare them using == in this case. Ideal would be to generate this warning only if the user code does this comparison but not for just compiling the header but I don't see how to do it.

comment:7 Changed 2 years ago by MortenMacFly

Well while the patch is nice, too and will silence the warning, I rather think my second proposal is mathematically more correct. It will make the function the best comparison (actually estimation) you can do with the double precision available. Just 2 cents from a math guy...

...whatever...

comment:8 Changed 2 years ago by vadz

We're not speaking about mathematics but computers here, please read something like this article (which I didn't read in details but it's one of the first matches in my web search and seems at least superficially correct) to appreciate the difference.

comment:9 follow-up: Changed 2 years ago by vadz

@jens, thank you for the patch but could we perhaps make it even better and more generally useful and define wxGCC_WARNING_SUPPRESS/RESTORE() macros like the ones described at the end of this article?

Changed 2 years ago by jens

silent warning about comparison of doubles with equality operator in wxIsSameDouble

comment:10 in reply to: ↑ 9 Changed 2 years ago by jens

Replying to vadz:

@jens, thank you for the patch but could we perhaps make it even better and more generally useful and define wxGCC_WARNING_SUPPRESS/RESTORE() macros like the ones described at the end of this article?

I just attached a renewed patch based on the macro in the article.

I put the macro into defs.h, but this might not be correct.
Not (yet) tested with gcc < 4.7.2 and only tested on linux 64-bit.

comment:11 Changed 2 years ago by vadz

Somehow not only this doesn't work for g++ < 4.7.2 (4.4.5 in my case), but it actually makes things worse by making the following warnings appear:

include/wx/any.h: In member function 'bool wxAny::operator==(float) const':
include/wx/any.h:924: warning: comparing floating point with == or != is unsafe
include/wx/any.h: In member function 'bool wxAny::operator==(double) const':
include/wx/any.h:934: warning: comparing floating point with == or != is unsafe

even after adding the macros around this code! My hypothesis is that once the warning is explicitly enabled with this version of g++, it can't be ignored any more.

So I'll just check in the macros for g++ 4.6 and later, this should be good enough for many people already (including the OP) and for most of them soon.

Thanks!

comment:12 Changed 2 years ago by jens

The problem is, that on gcc < 4.6 the setting can not be stored and restored (push and pop), so it is turned off with SUPRESS and turned on with RESTORE.
To switch off the warning, the RESTORE must come after the end of the function, that means after the closing brace, nit directly after the return-statement.
If I surround the whole function with the macros it works fine (with gcc 4.7).

It still should be fixed if any possible and not just suppressed.

By the way, there are many other float-equal warnings, that show potential errors.

Jens

comment:13 follow-up: Changed 2 years ago by vadz

I realize this but unfortunately it didn't work for me with the original macro definitions even though I did the following:

  • include/wx/any.h

    diff --git a/include/wx/any.h b/include/wx/any.h
    index 6e38772..2651ce2 100644
    a b class wxAny 
    912912    WXANY_IMPLEMENT_INT_EQ_OP(wxLongLong_t, wxULongLong_t) 
    913913#endif 
    914914 
     915    wxGCC_WARNING_SUPPRESS(float-equal) 
     916 
    915917    bool operator==(float value) const 
    916918    { 
    917919        if ( !wxAnyValueTypeImpl<float>::IsSameClass(m_type) ) 
    class wxAny 
    932934                (wxAnyValueTypeImpl<double>::GetValue(m_buffer)); 
    933935    } 
    934936 
     937    wxGCC_WARNING_RESTORE(float-equal) 
     938 
    935939    bool operator==(bool value) const 
    936940    { 
    937941        if ( !wxAnyValueTypeImpl<bool>::IsSameClass(m_type) ) 

comment:14 Changed 2 years ago by VZ

  • Resolution set to fixed
  • Status changed from confirmed to closed

(In [73280]) Add wxGCC_WARNING_{SUPPRESS,RESTORE} macros and use them for -Wfloat-equal.

Suppress the warnings about comparing floating point values for equality in
wxWidgets headers when the user code is compiled with -Wfloat-equal (at least
when using g++ 4.6 or later).

Closes #14895.

comment:15 in reply to: ↑ 13 ; follow-up: Changed 2 years ago by jens

Replying to vadz:

This works with gcc4.7 on my fedora system.
I will see if I have an older gcc available on one of my vm's (most likely CentOS 5 has it) or on my server with debian stable (but it is a little bit slower than my working system).
I will post a feedback in any case.
Most likely tomorrow, today it's a little late.

Jens

comment:16 in reply to: ↑ 15 Changed 2 years ago by jens

Replying to jens:

I will see if I have an older gcc available on one of my vm's (most likely CentOS 5 has it) or on my server with debian stable (but it is a little bit slower than my working system).
I will post a feedback in any case.

According to the gcc 4.2 documentation the diagnostic pragmas are not intended to be used per line, but per project. They should come before any functions and datas, every other use (like we do it here) depends on the optimizer.
So using it for gcc >= 4.6 only is the correct way (for the macro).

A correct fix for the equaity operator would be better in any way.
But it might be hard to implement for all floats (small, near zero, large).

Note: See TracTickets for help on using tickets.