Great! +1 I've had the override keyword catch a number of bugs in other projects.
On Thu, Nov 24, 2016 at 1:33 AM, Even Rouault <[email protected]> wrote: > 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 > -- -- http://schwehr.org
_______________________________________________ gdal-dev mailing list [email protected] http://lists.osgeo.org/mailman/listinfo/gdal-dev
