Opened 10 years ago

Closed 4 years ago

Last modified 4 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 Changed 10 years ago by warmerdam

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 Changed 10 years ago by Mateusz Łoskot

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 Changed 10 years ago by Mateusz Łoskot

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

comment:4 Changed 10 years ago by Mateusz Łoskot

Resolution: wontfix
Status: assignedclosed

Giving up.

comment:5 Changed 8 years ago by Mateusz Łoskot

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 Changed 5 years ago by Jukka Rahkonen

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 Changed 5 years ago by Mateusz Łoskot

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 Changed 5 years ago by Even Rouault

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

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

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 Changed 4 years ago by Mateusz Łoskot

Resolution: wontfix
Status: reopenedclosed

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

comment:11 in reply to:  10 Changed 4 years ago by Even Rouault

Replying to mloskot:

The idea has not been accepted.

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

comment:12 Changed 4 years ago by Mateusz Łoskot

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 Changed 4 years ago by Kurt Schwehr

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 Changed 4 years ago by Mateusz Łoskot

@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?

comment:15 in reply to:  14 Changed 4 years ago by Even Rouault

@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 Changed 4 years ago by Mateusz Łoskot

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

ogr\ogr_srs_api.h(329):OGRErr CPL_DLL CPL_STDCALL OSRImportFromEPSG( OGRSpatialReferenceH, int );
ogr\ogr_srs_api.h(330):OGRErr CPL_DLL CPL_STDCALL OSRImportFromEPSGA( OGRSpatialReferenceH, int );
ogr\ogr_srs_api.h(349):OGRErr CPL_DLL CPL_STDCALL OSRExportToWkt( OGRSpatialReferenceH, char ** );
ogr\ogr_srs_api.h(350):OGRErr CPL_DLL CPL_STDCALL OSRExportToPrettyWkt( OGRSpatialReferenceH, char **, int);
ogr\ogr_srs_api.h(351):OGRErr CPL_DLL CPL_STDCALL OSRExportToProj4( OGRSpatialReferenceH, char **);
ogr\ogr_srs_api.h(365):OGRErr CPL_DLL CPL_STDCALL OSRSetAttrValue( OGRSpatialReferenceH hSRS,
ogr\ogr_srs_api.h(397):OGRErr CPL_DLL CPL_STDCALL OSRSetFromUserInput( OGRSpatialReferenceH hSRS, 
Last edited 4 years ago by Mateusz Łoskot (previous) (diff)
Note: See TracTickets for help on using tickets.