Opened 15 years ago

Closed 8 years ago

Last modified 8 years ago

#3153 closed enhancement (wontfix)

Define OGRErr as enumerated type

Reported by: Mateusz Łoskot Owned by: Mateusz Łoskot
Priority: low Milestone:
Component: OGR_SF Version: svn-trunk
Severity: normal Keywords:
Cc:

Description

This ticket is an enhancement request to re-define OGRErr as enumerated type, similarly to CPLErr.

Thanks to strong-typed definition of CPLErr, it is possible to use this type as an exception type in C++ client code. It can be safely thrown and caught as an exception of distinct type. Unfortunately, this technique is not possible with OGRErr because OGRErr is an alias to int.

Perhaps, upcoming 1.7.0 would be a good opportunity to re-define OGRErr. If clients use OGRErr consistently as they should and they don't assign any values that are out of range of OGR error code, nothing should break. IMHO, making OGRErr a strong and unique type would be more beneficial in long term, especially for C++ users.

Change History (16)

comment:1 by warmerdam, 15 years ago

Priority: normallow

I'm concerned that this could trigger an C ABI change on windows if there are any functions using OGRErr that are also using stdcall name decoration which embeds some information on the argument (and return?) types in the function symbol. If so, I would be inclined against this change.

comment:2 by Mateusz Łoskot, 15 years ago

I understand, though it's a pity.

It confirms my concerns about C++ API development being kept back and blocked by the fact C API uses common elements with C++ API. It would be better to have C API maintained as a separate library on top of C++ library, but it's probably too late for a revolution.

Anyway, you can expect me to attach again when I spot GDAL 2.0 on the horizon :-)

comment:3 by Mateusz Łoskot, 14 years ago

Owner: changed from warmerdam to Mateusz Łoskot
Status: newassigned

comment:4 by Mateusz Łoskot, 14 years ago

Resolution: wontfix
Status: assignedclosed

Giving up.

comment:5 by Mateusz Łoskot, 12 years ago

Resolution: wontfix
Status: closedreopened

As promised, I'm reminding this idea for the GDAL 2.0 which seems to be close. It looks more serious changes to the public API are being approved by the way of Call for discussion for "RFC38 - OGR Faster Open", so I imagine replacing the OGRErr as enum could be accepted too.

comment:6 by Jukka Rahkonen, 9 years ago

http://trac.osgeo.org/gdal/wiki/rfc38_ogr_faster_open is targeted to GDAL 2.0. Does it address this issue at the same?

comment:7 by Mateusz Łoskot, 9 years ago

I don't see anything related to this request in the wiki:rfc38_ogr_faster_open Means, it's still postponed and waiting for Frank/Even's final decision.

comment:8 by Even Rouault, 9 years ago

I think Frank initial comments should be addressed to understand the consequences of the change.

in reply to:  1 comment:9 by Mateusz Łoskot, 9 years ago

Replying to warmerdam:

I'm concerned that this could trigger an C ABI change on windows if there are any functions using OGRErr that are also using stdcall name decoration which embeds some information on the argument (and return?) types in the function symbol.

Yes, replacing #define with enum effectively changes type affecting prototype of any function taking it as a parameter. Types of enum and int have different mangling type codes (http://www.agner.org/optimize/calling_conventions.pdf).

Hence, I targeted this change for 2.0 where, presumably, ABI changes are allowed.

comment:10 by Mateusz Łoskot, 8 years ago

Resolution: wontfix
Status: reopenedclosed

The idea has not been accepted. Since, GDAL 2 is out, proposed change lost its potential opportunity. Closing.

in reply to:  10 comment:11 by Even Rouault, 8 years ago

Replying to mloskot:

The idea has not been accepted.

By whom ? Did you propose this change to the community ?

comment:12 by Mateusz Łoskot, 8 years ago

ABI concerns and release of GDAL 2.0, plus longevity of this ticket is enough to conlcude with lack of acceptance. IMO, the ticket itself make a sufficient proposal - changes would be trivial and, obviously, I would have implemented if it, if it was accepted.

comment:13 by Kurt Schwehr, 8 years ago

mloskot, I am in favor of such a change. There is r30136 from 4 weeks ago that has OGRErr as an enum if -DSTRICT_OGRERR_TYPE is set.

comment:14 by Mateusz Łoskot, 8 years ago

@goatbar Clearly, I've missed that changeset. Thanks for pointing me at it.

@rouault Somewhat, request in this ticket has been addressed. Shall change the status to fixed then?

in reply to:  14 comment:15 by Even Rouault, 8 years ago

@rouault Somewhat, request in this ticket has been addressed. Shall change the status to fixed then?

Lol, apparently I can't even remember what I did 4 weeks ago. Well, that's just available if a define is set, not in the default case.

Are we sure about FrankW's concern about "C ABI change on windows if there are any functions using OGRErr that are also using stdcall name decoration which embeds some information on the argument" ? Would probably be worth checking/testing. If there's no such C ABI issue, then switching by default to the enumeration would be OK for 2.1.0

If there's an ABI issue, that's a bit more tricky. Wondering if there couldn't have some tricks to have the C ABI use the #define and the C++ ABI using the enumeration... Not pretty...

comment:16 by Mateusz Łoskot, 8 years ago

Quick grep-based check reveals the following functions use stdcall convention and return OGRErr:

ogr\ogr_fromepsg.cpp(2135):OGRErr CPL_STDCALL OSRImportFromEPSG( OGRSpatialReferenceH hSRS, int nCode )
ogr\ogr_fromepsg.cpp(2294):OGRErr CPL_STDCALL OSRImportFromEPSGA( OGRSpatialReferenceH hSRS, int nCode )
ogr\ogr_srs_proj4.cpp(1441):OGRErr CPL_STDCALL OSRExportToProj4( OGRSpatialReferenceH hSRS, 
ogr\ogrspatialreference.cpp(631):OGRErr CPL_STDCALL OSRExportToPrettyWkt( OGRSpatialReferenceH hSRS, char ** ppszReturn,
ogr\ogrspatialreference.cpp(685):OGRErr CPL_STDCALL OSRExportToWkt( OGRSpatialReferenceH hSRS,
ogr\ogrspatialreference.cpp(854):OGRErr CPL_STDCALL OSRSetAttrValue( OGRSpatialReferenceH hSRS,
ogr\ogrspatialreference.cpp(2128):OGRErr CPL_STDCALL OSRSetFromUserInput( OGRSpatialReferenceH hSRS, 
Version 0, edited 8 years ago by Mateusz Łoskot (next)
Note: See TracTickets for help on using tickets.