[gdal-dev] Mass addition of 'override' keyword ?

[Kurt Schwehr]

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 <even.rouault at spatialys.com>
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
> gdal-dev at lists.osgeo.org
> http://lists.osgeo.org/mailman/listinfo/gdal-dev
>



-- 
--
http://schwehr.org
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osgeo.org/pipermail/gdal-dev/attachments/20161125/c2c111f2/attachment-0001.html>