Hi, I've a branch where I've added the 'override' keyword to virtual methods that are an override of a virtual method declared in a base class.
https://github.com/OSGeo/gdal/compare/trunk...rouault:add_override?expand=1 Of course, not completely manually, but at >90% with the help of clang-tidy. GCC >= 5.1 has a -Wsuggest-override warning that I've enabled in configure to find all places missed by tidy-clang (at least with the drivers I've enabled in that branch). And recent clang versions have a less powerful -Winconsistent-missing-override (enabled with -Wall I think) that warns when you have explicitly tagged an overriden method with 'override' but neglected to do so for other overriden methods of the same class. So new code should be override friendly. As we still support C++03 compilers, the following tricks are used : 1) If the compiler doesn't advertize C++11 support, we #define override to empty in cpl_port.h. But that only if -DGDAL_COMPILATION is defined (which is the case in the Unix and Windows makefiles used to build GDAL), so as to avoid messing the namespace of code that includes GDAL. 2) As we don't want 'override' to leak into installed headers to keep C++03 compat, then CPL_OVERRIDE is used in those files, instead of plain 'override'. It is a #define to override is the compiler is C++11 enabled, or #define to empty otherwise. As words may poorly reflect the reality of the code, here's the code: {{{ #ifdef __cplusplus #if HAVE_CXX11 || _MSC_VER >= 1600 /** To be used in public headers only. For non-public headers or .cpp files, * use override directly. */ # define CPL_OVERRIDE override #else /** To be used in public headers only. For non-public headers or .cpp files, * use override directly. */ # define CPL_OVERRIDE /* For GDAL source compilation only, ignore override if non C++11 compiler */ #ifdef GDAL_COMPILATION # define override #endif #endif /* __cpluscplus */ }}} So in summary: use override in all .cpp files and non-installed .h headers. Use CPL_OVERRIDE in installed .h headers. Will make C++11 compilers enforce overriding, and C++03 ones ignore it. Interestingly in the process, I found a rather odd bug which pre-existed unnoticed. It is Windows specific. In VSIFilesystemHandler we have a virtual method GetDiskFreeSpace(). But on Windows, if you include <windows.h>, GetDiskFreeSpace is a #define to the ANSI Win32 function GetDiskFreeSpaceA() ... So the VSIWin32FilesystemHandler::GetDiskFreeSpace() method was actually evaluated as VSIWin32FilesystemHandler::GetDiskFreeSpaceA() by the preprocessor in port/cpl_vsil_win32.cpp, and not as an override of the base VSIFilesystemHandler::GetDiskFreeSpace()... Not critical since this is rarely used, and code that use this method should be ready to receive the default -1 value for filesystems that don't implement it. Thoughts regarding committing this? Windows and Unix builds are happy: * https://ci.appveyor.com/project/rouault/gdal2/build/1.0.38 (this one is C++11. Well actually VS12 is partially C++11, but sufficiently to understand override) * https://travis-ci.org/rouault/gdal2/builds/178478253 (mix of C++11 and C++03 configs) Even ~~~~~~~~~~~~~~~~~ PS. For reference, here's the method I've rougly used to convert (with clang-tidy of clang 3.8) Create in GDAL root directory a ".clang-tidy" file with the following content: { Checks: '-*,modernize-use-override' } Then create a Python script "apply.py" that will use the output of clang-tidy to patch files with override. There is some complication to avoid it to remove the "virtual" keyword while adding the "override" one (and also avoiding tagging virtual destructors as override, which doesn't bring anything interesting. And actually is disliked by VS 2010). I think that for some analyzers like cppcheck, the virtual keyword might help them better understand the logic when they don't understand the override keyword (cppcheck 1.72 is in fact confused by the override keyword and produces a lot of false positives, so in the cppcheck.sh script, override is forced to be an empty #define). {{{ import yaml import sys root = yaml.load( open(sys.argv[1], 'rb').read() ) replacements = root['Replacements'] files = {} for rep in replacements: f = rep['FilePath'] if not f in files: files[f] = [] files[f].append(rep) for f in files: content = open(f).read() shift = 0 for rep in files[f]: # Do not apply replacements that will remove the 'virtual' keyword if rep['Length'] == 0 and rep['ReplacementText'] == ' override': insert_pos = rep['Offset'] + shift # Adding override to a virtual destructor is quite ugly. Don't do it is_destructor = False for i in range(80): if i > insert_pos: break if content[insert_pos-1-i] == ';': break if content[insert_pos-1-i] == '~': is_destructor = True break if not is_destructor: content = content[0:insert_pos] + rep['ReplacementText'] + content[insert_pos:] shift += len(rep['ReplacementText']) open(f, 'wb').write(content) }}} And then the main script "modernize.py" {{{ import os import glob # To patch all drivers below frmts/. Adapt for other directories for dirname in glob.glob('frmts/*'): for file in glob.glob(dirname + '/*.h*') + glob.glob(dirname + '/*.cpp'): for file in glob.glob(dirname + '/*.h*') + glob.glob(dirname + '/*.c*'): print file os.system('rm replace.yml') os.system('clang-tidy -export-fixes=replace.yml ' + file + ' -- -Iogr/ogrsf_frmts/gpkg -Ifrmts/vrt -I/usr/include/postgresql/ -Ifrmts/wms -I/usr/include -I/usr/include/xercesc -Ifrmts/netcdf -Ifrmts/pcidsk/sdk -Ifrmts/pcidsk/sdk/core -Ifrmts/pcidsk/sdk/channel -Ifrmts/gtiff -DLERC -Ifrmts/mrf -Ifrmts/mrf/libLERC -Ifrmts/raw -Iogr/ogrsf_frmts/sqlite -Iogr/ogrsf_frmts/couchdb -Iogr/ogrsf_frmts/geojson -Iogr/ogrsf_frmts/geojson/libjson -I/usr/include/fyba -Iogr/ogrsf_frmts/shape -Iogr/ogrsf_frmts/pgeo -Iogr/ogrsf_frmts/pgdump -Iogr/ogrsf_frmts/xplane -I/usr/include/ogdi -Iogr/ogrsf_frmts/gml -Iogr/ogrsf_frmts/mem -Iogr/ogrsf_frmts/gtm -Iogr/ogrsf_frmts/kml -Iogr/ogrsf_frmts/generic -DHAVE_POPPLER -I/usr/include/poppler -DHAVE_PODOFO -I/usr/include/podofo -DDEBUG -DCPL_UNUSED -DHAVE_SQLITe -DHAVE_XERCES -DHAVE_EXPAT -DHAVE_CURL -DHAVE_CRYPTOPP -DFRMT_ecw -Ignm -Ignm/gnm_frmts -I/home/even/install-libecwj2-3.3/include -I/usr/include/c++/5 -I/home/even/FileGDB_API_1.4/include -I/usr/include/x86_64-linux-gnu/c++/5 -Iport -Igcore -Iogr -Iogr/ogrsf_frmts -Ialg -std=c++11') os.system('python apply.py replace.yml') }}} Build, fix errors, and run "make install" and grep override in the installed headers. Then use your favorite text editor to replace override by CPL_OVERRIDE in those few files. -- Spatialys - Geospatial professional services http://www.spatialys.com
_______________________________________________ gdal-dev mailing list [email protected] http://lists.osgeo.org/mailman/listinfo/gdal-dev
