Make new method in OGREnvelope following const-correctness

[Mateusz Łoskot]

There have been new methods added to the OGREnvelope (r11894). I'd like to emphasise it should be important to try to make new API designed more carefully with const-correctness in mind.

My suggestion is to change prototypes of these two new methods to:

int Intersects(OGREnvelope const& other);
int Contains(OGREnvelope const& other);

At early stage, such changes are harmless and completely transparent for existing code, but after some time, fixing code to follow const-correctness may be impossible. So, if we make it sooner then it's better.

[Mateusz Łoskot]

I'm assigning this ticket to rouault, author of the patch.

[Even Rouault]

Done in r11899. I've been even more pedantic about const correctness.

int Intersects(OGREnvelope const& other) const;
int Contains(OGREnvelope const& other) const;

In fact, I think it could also possible to retrofit the API of the 2 other methods (not done in this commit), IsInit and Merge, because as they are small and defined in a .h, they are always inlined by the compiler at -O2 (I've searched for OGREnvelope symbols in libgdal.so and found none). Of course they would be the risk of someone using these methods outside of the GDAL source tree, GDAL being compiled with -O0...

[Even Rouault]

Oh, by the way, Mateusz, you call me by my first name, which is Even ;-)

[Mateusz Łoskot]

Thanks! I apologize for using your nickname but I didn't know your real name. Forgive me my ignorance, I should check the COMMITERS file first.

(re const-correctness) I think it's a good idea. It should not break any of existing and users code, as it's valid to call const function using non-const pointer or reference (but not the other way, certainly), what I'm sure occurs in current code very often.

Summarizing, I vote +1

[Even Rouault]

In r11900, I've modified the signature of IsInit and Merge to :

int  IsInit() const;
void Merge( OGREnvelope const& sOther );

After a bit of investigation, I think this can be made safely because the methods are defined in the class declaration. Thus, even if you call them outside GDAL, the compiler can't be sure that the symbols for those methods exist in the GDAL library. So it must recompile them for the calling code. Of course, if those methods had been only declared in ogr_core.h and defined somewhere else in GDAL, a symbol would have been created in GDAL and changing the signature of the methods would have cause a change in symbol name and thus breaking ABI. So all in all, AFAIK, we are in the rare situation where we can change the API without breaking the ABI. I've always thought that C++ is a nightmare when it comes to ABI stability...

[warmerdam]

I'll confirm that this const correctness change is acceptable (in trunk), but I've been burned by ABI changes due to const correctness changes in the past, and so I prefer to be very restrictive about such changes. I would discourage a "const correctness crusade", at least without an RFC.

[Mateusz Łoskot]

Replying to warmerdam:

I would discourage a "const correctness crusade", at least without an RFC.

Frank,

I started the crusade but only as a recommended practice when adding new functions.