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

[Even Rouault]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osgeo.org/pipermail/gdal-dev/attachments/20161124/67af96c6/attachment-0001.html>