Tweak OGREnvelope IsInit to not do -Wfloat-compare

[hobu]

Just about every operation of OGREnvelop checks for IsInit, which in turn checks each of the four positions whether or not they are equal 0.0 by using a float comparision. This makes -Wfloat-equal whine when used.

One action is to simply pragma around the code:

#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wfloat-equal"
    int  IsInit() const { return MinX != 0 || MinY != 0 || MaxX != 0 || MaxY != 0; }
#pragma GCC diagnostic pop

but this isn't so satisfactory as it mucks things up and papers over things.

Can we instead add a bool to OGREnvelope that states whether or not the instance has been instantiated, and have IsInit simply check the boolean? That would remove the 4 comparisons for most operations. I'd even be willing to beef up OGREnvelope to contain more operations similar to PDAL's if you're interested. I'm willing to do whatever you want to do, but it would be nice to shut this error up.

[hobu]

See patch for proposed attempt at what was described in the ticket.

[Even Rouault]

OGREnvelope is also defined as a C structure, so the addition of the boolean in the C++ class could cause issues if C code provides the C struct (for example OGR_G_GetEnvelope ) and OGR C++ code handles it as the C++ class.

[warmerdam]

I'm against changing the binary format of the OGREnvelope without very careful thought and a compelling reason.

Is this some sort of warning about doing floating point comparisons to zero? I don't see what that wouldn't be reliable. IF it's a quirk of one compiler, I'm fine with masking it with pragmas.

[hobu]

It's a quirk of clang and gcc 4.4+ when the -Wfloat-equal warning is turned on. I guess I didn't look very close that OGREnvelope is a C++ class and just a struct in C land.

Another option might be to do epsilon comparisons instead of simple equality tests, but I know Frank doesn't like that approach to simply satisfy a whiny warning.

I'll slap some pragmas around it with a note pointing to here.

[hobu]

Fix to ignore the warning, and not change the struct, applied in r26737.

[Even Rouault]

The new pragma generates a "c:\utilisateurs\even\gdal\ogr\ogr_core.h(69) : warning C4068: unknown pragma" for each OGR .cpp file compiled with Visual Studio 2010

[hobu]

Another try in r26741 so as to not have windows complain

[Even Rouault]

r26741 has fixed the MSVC warning. Thanks

[hobu]

Still too noisy for GCC's <= 4.6

[hobu]

Another attempt to shut this up in r26826.

[Kyle Shannon]

Howard, I'm still getting warnings from this on GCC 4.6.3 on Ubuntu 12.04 x86_64:

home/kyle/src/gdal/trunk/gdal/ogr/ogr_core.h:72:32: warning: missing option after '#pragma GCC diagnostic' kind [-Wpragmas]

Is anyone else still getting this warning?

The code:

#if (__GNUC__ == 4 && __GNUC_MINOR__ >= 6 && !defined(_MSC_VER)) 
#pragma GCC diagnostic ignored "-Wfloat-equal"
#pragma GCC diagnostic push
#endif
    int  IsInit() const { return MinX != 0 || MinY != 0 || MaxX != 0 || MaxY != 0; }

#if (__GNUC__ == 4 && __GNUC_MINOR__ >= 6 && !defined(_MSC_VER))
#pragma GCC diagnostic pop
#endif

Silences it for me.

[Kyle Shannon]

The pragma as defined doesn't make any sense to me, as per:

​http://gcc.gnu.org/onlinedocs/gcc/Diagnostic-Pragmas.html

You have to queue up a diagnostic, then push it, then pop it. I think the code guards are fine, but the pragma wasn't working. Fixed in r27058.

[Kyle Shannon]

Closing.

[simonsonc]

Replying to kyle:

The pragma as defined doesn't make any sense to me, as per:

​http://gcc.gnu.org/onlinedocs/gcc/Diagnostic-Pragmas.html

You have to queue up a diagnostic, then push it, then pop it. I think the code guards are fine, but the pragma wasn't working. Fixed in r27058.

Shouldn't the push be first, then the ignore? @kyle, wasn't the problem you were seeing that the "pop" mistakenly had an "ignored" before it?

[Kyle Shannon]

According to the documentation, no. If you have a different take, I'd be more than happy to take a look. My interpretation is that diagnostics can be queued, then pushed and popped at will, and when there is an invalid pop, it reverts to the original command line args. What would a push do with nothing queued to be pushed. If I'm mistaken, reopen please.

[Even Rouault]

trunk r27791 and branches/1.11 r27792 "ogr_core.h: only ignore -Wfloat-equal for IsInit() and not for the rest of the file and files that inc it (#5299)"

I also agree with simonsonc that ignored should be placed between push and pop. The GCC doc that you mention shows that as an example too

And if you compile the following snippet with -Wfloat-equal, you'll see that boths if() tests are silented, instead of the first one. But if you put ignored after push, then only the first one is silented.

#include <stdio.h>

int main(int argc, char* argv[])
{
    double f = 0.0;
#if ((__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 6)) && !defined(_MSC_VER)) 
#pragma GCC diagnostic ignored "-Wfloat-equal"
#pragma GCC diagnostic push
#endif
    if( f == 0.0 )
        printf("foo\n");
#if ((__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 6)) && !defined(_MSC_VER)) 
#pragma GCC diagnostic pop
#endif
    if( f == 0.0 )
        printf("foo\n");
    return 0;
}

[Kyle Shannon]

My mistake.