[Gdal-dev] RFC 10: OGR Open Parameters

[Daniel Morissette]

Andrey Kiselev wrote:
> Daniel,
> 
> On Tue, Mar 13, 2007 at 10:03:44AM -0400, Daniel Morissette wrote:
>> Do we have an idea of how many third party OGR drivers exist that
>> would be impacted by that change? (i.e. not counting the drivers that
>> are in the GDAL SVN repository).
> 
> This change is unavoidable, because OGRSFDriver::Open() is pure virtual
> method. Fortunately it is pretty trivial to fix the driver, so I do not
> see any serious problem for third-party drivers, if such exist.
> 

If no third party drivers exist then perhaps backwards compatibility is 
a non-issue, but your RFC refers to third party drivers, so I assume 
that you meant by that that some do exist. That's why I asked for a 
number... to estimate the impact of breaking backwards compatibility.


>> Actually, could we not avoid any backwards compatibility issue by 
>> playing with the fact that the three Open() methods are virtual?
>>
>> The base classes could define a default implementation for the new 
>> method that just maps the call to the old method without the 
>> papszOptions, e.g.
>>
>> OGRDataSource *
>> OGRSFDriverRegistrar::Open( const char * pszName, int bUpdate,
>>                             OGRSFDriver ** ppoDriver,
>> 			    char **papszOptions /*= NULL*/ )
>> {
>>     /* Map this call to the old/deprecated GDAL 1.4.0 Open() method
>>      * for drivers that do not implement the new Open() method added
>>      * in GDAL 1.5.0+
>>      */
>>     return Open( pszName, bUpdate, ppoDriver );
>> }
>>
>>
>> If I'm not mistaken, this way old drivers would continue to work even if 
>> they do not define the new Open() method... and upgraded drivers that 
>> override the new Open() method could take advantage of the new 
>> papszOptions feature.
> 
> Sorry, but I do not understand the point here. OGRSFDriver::Open() is
> the method that should be fixed in drivers because it is pure virtual.

Sorry, I used the wrong class for my example. The approach that I 
suggested should be applied to OGRSFDriver::Open() then.


> How new OGRSFDriver::Open() should be declared? Should it be pure
> virtual as the old method? Then drivers will need to implement both old
> and new ones. If it will not be a pure virtual, then drivers still need
> to implement the old Open() method, even if they want to go with new
> style Open() call only. Please, correct me if I wrong.
> 

I just realized the problem you may be referring to. I think the 
solution is that both versions of OGRSFDriver::Open() should be virtual 
methods (but NOT pure virtual) and the default implementations would be:

OGRDataSource
OGRSFDriver::*Open( const char *pszName, int bUpdate=FALSE,
                     char **papszOptions /*= NULL*/ )
{
     /* Map this call to the old/deprecated GDAL 1.4.0 Open() method
      * for drivers that do not implement the new Open() method added
      * in GDAL 1.5.0+
      */
     return Open( pszName, bUpdate );
}

OGRDataSource
OGRSFDriver::*Open( const char *pszName, int bUpdate=FALSE )
{
     /* This method is deprecated. Return NULL by default.
      * If a driver implements this virtual method then the driver's
      * implementation will be called instead
      */
     return NULL;
}


Then OGR drivers are required to implement only one of the two methods. 
Leading to two possible scenarios:

1- Driver implements old Open() method. Calls to the new method are 
automatically rerouted to the driver's implementation of the old Open() 
by the OGRSFDriver::*Open( const char *pszName, int bUpdate=FALSE, char 
**papszOptions /*= NULL*/ ) implementation above.

2- Driver implements the new Open() method. Since the method is virtual, 
the driver's implementation takes precedence over the default 
implementation and the driver's own Open() is called.

Of course for this to work it is assumed that OGR will never attempt to 
call the old Open()... i.e. that it will always pass the papszOptions 
argument to Open() calls.  Um... perhaps that's where my proposal 
breaks.  :(

Daniel
-- 
Daniel Morissette
http://www.mapgears.com/