Conflicting ResetReading virtual method defs

[Kurt Schwehr]

I am thinking that the ogrsf_frmts drivers should have:

• virtual (or override or final with C++ 11 or newer) for the ResetReading() method.
• return type of void (there a couple int returns like osm)
• should never take an arg (vfk takes an arg)

Is this correct or am I missing something?

// grep ResetReading *.h */*.h
ogr_api.h:void   CPL_DLL OGR_L_ResetReading( OGRLayerH );
ograpispy.h:void OGRAPISpy_L_ResetReading( OGRLayerH hLayer );
ogrsf_frmts/ogrsf_frmts.h:    virtual void        ResetReading() = 0;

find . -name \*.h -o -name \*.c\* | xargs egrep 'ResetReading *[(]' | grep void | wc -l
     335
find . -name \*.h -o -name \*.c\* | xargs egrep 'ResetReading *[(]' | grep 'int ' | wc -l
       6

Are these bad? Should the int returns be void and should vfk's really take an arg?

find . -name \*.h -o -name \*.c\* | xargs egrep 'ResetReading *[(]' | grep 'int ' 
./ogrsf_frmts/geojson/ogrgeojsondriver.cpp:        int ResetReading();
./ogrsf_frmts/geojson/ogrgeojsondriver.cpp:int OGRESRIFeatureServiceDataset::ResetReading()
./ogrsf_frmts/osm/ogr_osm.h:    int                 ResetReading();
./ogrsf_frmts/osm/ogrosmdatasource.cpp:int OGROSMDataSource::ResetReading()
./ogrsf_frmts/vfk/vfkdatablock.cpp:void IVFKDataBlock::ResetReading(int iIdx)
./ogrsf_frmts/vfk/vfkreader.h:    void               ResetReading(int iIdx = -1);

Shouldn't they all be virtual or all not virtual?

find . -name \*.h -o -name \*.c\* | xargs egrep 'ResetReading *[(]' | grep void | grep virtual | wc -l
     118
  
find . -name \*.h -o -name \*.c\* | xargs egrep 'ResetReading *[(]' | grep void | grep -v virtual | wc -l
     217

find . -name \*.h -o -name \*.c\* | xargs egrep 'ResetReading *[(]' | grep void | grep -v virtual | head -15
./ogr_api.h:void   CPL_DLL OGR_L_ResetReading( OGRLayerH );
./ograpispy.cpp:void OGRAPISpy_L_ResetReading( OGRLayerH hLayer ) { OGRAPISpy_L_Op(hLayer, "ResetReading"); }
./ograpispy.h:void OGRAPISpy_L_ResetReading( OGRLayerH hLayer );
./ogrsf_frmts/aeronavfaa/ograeronavfaalayer.cpp:void OGRAeronavFAALayer::ResetReading()
./ogrsf_frmts/aeronavfaa/ograeronavfaalayer.cpp:void OGRAeronavFAARouteLayer::ResetReading()
./ogrsf_frmts/aeronavfaa/ograeronavfaalayer.cpp:void OGRAeronavFAAIAPLayer::ResetReading()
./ogrsf_frmts/amigocloud/ogramigocloudlayer.cpp:void OGRAmigoCloudLayer::ResetReading()
./ogrsf_frmts/arcgen/ograrcgenlayer.cpp:void OGRARCGENLayer::ResetReading()
./ogrsf_frmts/arcobjects/aolayer.cpp:void AOLayer::ResetReading()
./ogrsf_frmts/avc/ogr_avc.h:    void                ResetReading();
./ogrsf_frmts/avc/ogr_avc.h:    void                ResetReading();
./ogrsf_frmts/avc/ogravcbinlayer.cpp:void OGRAVCBinLayer::ResetReading()
./ogrsf_frmts/avc/ogravce00layer.cpp:void OGRAVCE00Layer::ResetReading()
./ogrsf_frmts/bna/ogr_bna.h:    void                ResetReading();
./ogrsf_frmts/bna/ogrbnalayer.cpp:void OGRBNALayer::ResetReading()

find . -name \*.h -o -name \*.c\* | xargs egrep 'ResetReading *[(]' | grep void | grep virtual | head -15
./ogrsf_frmts/aeronavfaa/ogr_aeronavfaa.h:    virtual void                ResetReading();
./ogrsf_frmts/aeronavfaa/ogr_aeronavfaa.h:    virtual void                ResetReading();
./ogrsf_frmts/aeronavfaa/ogr_aeronavfaa.h:    virtual void                ResetReading();
./ogrsf_frmts/amigocloud/ogr_amigocloud.h:        virtual void                ResetReading();
./ogrsf_frmts/arcgen/ogr_arcgen.h:    virtual void                ResetReading();
./ogrsf_frmts/arcobjects/ogr_ao.h:  virtual void        ResetReading();
./ogrsf_frmts/carto/ogr_carto.h:    virtual void                ResetReading();
./ogrsf_frmts/couchdb/ogr_couchdb.h:    virtual void                ResetReading();
./ogrsf_frmts/couchdb/ogr_couchdb.h:    virtual void                ResetReading();
./ogrsf_frmts/couchdb/ogr_couchdb.h:    virtual void                ResetReading();
./ogrsf_frmts/couchdb/ogrcouchdbdatasource.cpp:        virtual void        ResetReading() { bEnd = false;}
./ogrsf_frmts/csw/ogrcswdataset.cpp:    virtual void                ResetReading();
./ogrsf_frmts/db2/ogr_db2.h:    virtual void        ResetReading();
./ogrsf_frmts/db2/ogr_db2.h:    virtual void        ResetReading();
./ogrsf_frmts/db2/ogr_db2.h:    virtual void        ResetReading();

[Kurt Schwehr]

I'm not seeing a failure from this. Just confused by what I saw while looking at cleaning the osm driver

[Even Rouault]

This should be addressed, at least partially, as a side effect of ​https://trac.osgeo.org/gdal/wiki/rfc66_randomlayerreadwrite

Not sure if you noticed, in your search, you get ResetReading() methods from classes that don't derive from OGRLayer.

And regarding virtual vs non-virtual, as soon as the base virtual method is declared virtual, any overriden method of the same name and signature in a derived class is necessary virtual, even if it doesn't mention it explicitly. The C++11 override keyword would be more approrpriate in those case to underline and enforce the overriding of a base virtual method.

[Even Rouault]

I think this ticket can now be closed now that RFC66 has been merged ?