Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#5299 closed defect (fixed)

Tweak OGREnvelope IsInit to not do -Wfloat-compare

Reported by: hobu Owned by: warmerdam
Priority: normal Milestone: 1.11.2
Component: default Version: unspecified
Severity: normal Keywords:
Cc: Kyle Shannon

Description

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.

Attachments (1)

OGREnvelope.patch (1.7 KB) - added by hobu 8 years ago.

Download all attachments as: .zip

Change History (20)

Changed 8 years ago by hobu

Attachment: OGREnvelope.patch added

comment:1 Changed 8 years ago by hobu

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

comment:2 Changed 8 years ago by 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.

comment:3 Changed 8 years ago by 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.

comment:4 Changed 8 years ago by 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.

comment:5 Changed 8 years ago by hobu

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

comment:6 Changed 8 years ago by hobu

Milestone: 1.11.0
Resolution: fixed
Status: newclosed

comment:7 Changed 8 years ago by 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

comment:8 Changed 8 years ago by hobu

Another try in r26741 so as to not have windows complain

comment:9 Changed 8 years ago by Even Rouault

r26741 has fixed the MSVC warning. Thanks

comment:10 Changed 8 years ago by hobu

Resolution: fixed
Status: closedreopened

Still too noisy for GCC's <= 4.6

comment:11 Changed 8 years ago by hobu

Another attempt to shut this up in r26826.

comment:12 Changed 8 years ago by 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.

comment:13 Changed 8 years ago by Kyle Shannon

Cc: Kyle Shannon added

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.

comment:14 Changed 8 years ago by Kyle Shannon

Resolution: fixed
Status: reopenedclosed

Closing.

comment:15 in reply to:  13 Changed 8 years ago by 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?

comment:16 Changed 8 years ago by 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.

comment:17 Changed 7 years ago by 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;
}

comment:18 Changed 7 years ago by Even Rouault

Milestone: 1.11.01.11.2

comment:19 Changed 7 years ago by Kyle Shannon

My mistake.

Note: See TracTickets for help on using tickets.