Request for Comments-36: Move OTB to C++14

From OTBWiki
Jump to: navigation, search

[Request for Comments - 36] Move OTB to C++14

Status

  • Author: Jordi Inglada
  • Submitted on 09.06.2017
  • Open for comments

Content

What changes will be made and why they would make a better Orfeo ToolBox?

Motivation

OTB uses the C++98/03 ISO standard, which was the one available at the beginning of the project. The C++11 ISO standard brings many features allowing for simpler, cleaner, safer and even better performing code. C++14 brings smaller features, yet useful. See for instance: [1], [2], [3], [4] and [5].

Specific examples for OTB can be found here [6].

What Moving to C++14 means

The move towards can be performed at different levels with different implications to the project. From shallow to deep they are:

  1. Allow users of the library to use C++14. This needs that OTB builds correctly when the compiler uses the C++14 standard. This is the case currently.
  2. Allow new code to use the C++14 standard. This means that OTB only compiles with C++14, and therefore that a recent C++ compiler is used. This should not be a problem as one can see here [7].
  3. Modify the existing OTB code to use C++14 features.This could be a lot of work (but some automation can be achieved [8]) with limited usefulness. Although one can argue that this would invite developers to use a modern style when making evolutions to existing code if they find a modern style already.

This RFC aims at validating a plan to achieve the 2 last points above.

Proposed plan
Compile OTB with C++14 by default

This can be achieved by using the following CMake commands:

set(CMAKE_CXX_STANDARD 14)
set(CMAKE_CXX_STANDARD_REQUIRED ON)
set(CMAKE_CXX_EXTENSIONS OFF)

The current mechanism for modules needing c++11 becomes obsolete for c++<=14

Transform OTB code with clang-tidy

clang-tidy proposes many modernize checkers:

  • modernize-avoid-bind
  • modernize-deprecated-headers
  • modernize-loop-convert
  • modernize-make-shared
  • modernize-make-unique
  • modernize-pass-by-value
  • modernize-raw-string-literal
  • modernize-redundant-void-arg
  • modernize-replace-auto-ptr
  • modernize-shrink-to-fit
  • modernize-use-auto
  • modernize-use-bool-literals
  • modernize-use-default
  • modernize-use-emplace
  • modernize-use-nullptr
  • modernize-use-override
  • modernize-use-using

An experimental branch applying some of them (deprecated-headers, loop-convert, use-auto, use-bool-literals, use-nullptr, use-override) to OTB core, tests and examples is available [9]. It builds successfully with clang-3.9 and gcc-6.3. It should be added to the tested branches in the dashboard.

To run clang-tidy on a complete CMake project, on has to run CMake with -DCMAKE_EXPORT_COMPILE_COMMANDS=ON in order to generate a JSON file which can be used by the run-clang-tidy script. Then, one can do:

run-clang-tidy-3.9.py -header-filter='.*' -checks='-*,modernize-use-auto,modernize-loop-convert,modernize-deprecated-headers,modernize-use-override,modernize-use-nullptr,modernize-use-bool-literals' -fix

Before running clang-tidy, it is useful to replace some ITK macros like ITK_NULLPTR and ITK_OVERRIDE by the standard keywords nullptr and override:

find ./ -type f -print0 | xargs -0 sed -i 's/ITK_OVERRIDE/override/g'
find ./ -type f -print0 | xargs -0 sed -i 's/ITK_NULLPTR/nullptr/g'

After running clang-tidy, 2 small corrections have to be made:

find ./ -type f -print0 | xargs -0 sed -i 's/itkFactorylessNewMacro(auto)/itkFactorylessNewMacro(Self)/g'
find ./ -type f -print0 | xargs -0 sed -i 's/auto char/auto/g'

The modernize-use-using checker finds conflicts and does not apply the fixes. I don't have a solution for that other than doing the transformation with an ad-hoc script.

The use of other checkers should be discussed.

Set the infrastructure for periodic static checking of the code

In order to ensure that new OTB code uses C++14, an automatic checking using clang-tidy could be implemented. Also, CPP Core Guidelines checkers could be activated.

CMake supports the use of clang-tidy. See [10].

This should also be discussed.

When will those changes be available (target release or date)?

Those changes can be available for release 6.2.

Who will be developing the proposed changes?

Community

Comments

List here important comments (possibly links) received.

"Compile OTB with C++14 by default" as soon as possible

so that all 6.2 binary packages are built with C++14 (but still almost C++98 compatible) so we can test an entire release cycle for the compatibility of compilers, OS, dashboard, packaging, etc... Then if all is well we can be aggressive with clang-tidy in the 6.4 RFC window. [11]

And add "= delete" to a core OTB file as a force-check for our infrastructure. [12]

Impact on project using OTB

Do we have an idea of the impact on other projects using OTB ? I am guessing they will have the choice to either jump to C++14, or stay on a stable branch like release-6.0 where we can provide minor releases if needed. [13]

Impact on Linux packaging

What is the impact on Linux packaging ? At the moment we use gcc 4.8, but we will have to solve the mess with glib/freetype/expat stuff before distributing binaries built with gcc 5 (for c++14). [14]

Drop REQUIRE_CXX11

Can we drop the specific REQUIRE_CXX11 in our modules ? I think yes. Do we need a different parameter, like a "Minimum C++ standard", for future modules that will need C++17 or later ? [15]

Remaining issues after clang-tidy modernize

Luc has provided the following analysis.

I took the opportunity to check some other patterns that seems to have been either missed by the modernization (C-, D-), or that need a little bit more work to be really modernized (A-, B-).

Using https://github.com/LucHermitte/lh-misc/blob/master/bin/searchfile.pl we find:

A- searchfile.pl -e h,cxx,hxx "\<new\>" -c | less -R and I see quite number of `auto * ptr= new type[nbelems]` Can't we replace them with `std::vector<type>` ? Or even std::unique_ptr<type[]>

B- searchfile.pl -e h,cxx,hxx "\<delete\>" -c | less -R and I see too many entries. One of the main advantage of C++11: we should be able to get rid of `delete` and have a more robust code. One of the main advantage of C++14: we should be able to get rid of `new`

C- searchfile.pl -e h,cxx,hxx "\<for auto.*begin\>" -c | less -R - ./Modules/Adapters/GdalAdapters/include/otbOGRHelpers.h

 - for (auto b = boost::begin(strings), e = boost::end(strings); b != e; ++b)
 -    {
 -    m_raw.push_back(b->c_str());
 -    }
 + for (auto && str : strings)
 +    {
 +    m_raw.push_back(str.c_str());
 +    }

The same applies to

- ./Modules/Core/Metadata/src/otbQuickBirdImageMetadataInterface.cxx:
for (auto it = rawBandNames.begin(); it != rawBandNames.end(); ++it)
- ./Modules/Core/Metadata/src/otbWorldView2ImageMetadataInterface.cxx:
for (auto it = rawBandNames.begin(); it != rawBandNames.end(); ++it)
- ./Modules/Core/Metadata/src/otbSpot6ImageMetadataInterface.cxx:
for (auto it = rawBandNames.begin(); it != rawBandNames.end(); ++it)
- ./Modules/Core/Metadata/src/otbPleiadesImageMetadataInterface.cxx:
for (auto it = rawBandNames.begin(); it != rawBandNames.end(); ++it)
- ./Modules/Core/LabelMap/include/otbAttributesMapLabelObject.h:
for (auto it = m_Attributes.begin();
- ./Modules/Applications/AppImageUtils/app/otbDownloadSRTMTiles.cxx:
for(auto it= tiles.begin(); it!=tiles.end(); ++it)
- ./Modules/Applications/AppSegmentation/app/otbLSMSSmallRegionsMerging.cxx:
for(auto itAdjLabel=(adjMap[curLabel]).begin();
- ./Modules/Detection/RoadExtraction/test/otbLinkPathListFilter.cxx:
for (auto it = MatricePoint.begin(); it != MatricePoint.end(); ++it)
- ./Modules/Detection/RoadExtraction/test/otbBreakAngularPathListFilter.cxx:
for (auto it = MatricePoint.begin(); it != MatricePoint.end(); ++it)
- ./Modules/Detection/RoadExtraction/test/otbRemoveTortuousPathListFilter.cxx:
for (auto it = MatricePoint.begin(); it != MatricePoint.end(); ++it)
- ./Modules/Detection/RoadExtraction/test/otbSimplifyPathListFilter.cxx:
for (auto it = MatricePoint.begin(); it != MatricePoint.end(); ++it)
- ./Modules/Detection/RoadExtraction/test/otbLikelihoodPathListFilter.cxx:
for (auto it = MatricePoint.begin(); it != MatricePoint.end(); ++it)
- ./Modules/IO/IOXML/test/otbStatisticsXMLFileWriteAndRead.cxx:  
for (auto it = map.begin() ; it != map.end() ; ++it)

D- searchfile.pl -e h,cxx,hxx "//.* not implemented" -c | less -R and I see many files that should have used ITK_DELETED_FUNCTION, and that should use "= delete;", or even better: disappear in child classes where the default implicitly generated destructor works well.

Support

List here community members that support this RFComments.

Corresponding Requests for Changes

  1. Request for Changes-95: Compile OTB with C++14 by default (proposed)