Opened 16 years ago

Closed 16 years ago

Last modified 16 years ago

#1752 closed enhancement (fixed)

Make new method in OGREnvelope following const-correctness

Reported by: Mateusz Łoskot Owned by: Even Rouault
Priority: normal Milestone:
Component: default Version: svn-trunk
Severity: normal Keywords: const


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.

Change History (7)

comment:1 by Mateusz Łoskot, 16 years ago

Owner: changed from warmerdam to Even Rouault

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

comment:2 by Even Rouault, 16 years ago

Resolution: fixed
Status: newclosed

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 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...

comment:3 by Even Rouault, 16 years ago

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

comment:4 by Mateusz Łoskot, 16 years ago

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

comment:5 by Even Rouault, 16 years ago

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...

comment:6 by warmerdam, 16 years ago

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.

in reply to:  6 comment:7 by Mateusz Łoskot, 16 years ago

Replying to warmerdam:

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


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

Note: See TracTickets for help on using tickets.