Difference between revisions of "Documentation/Nightly/Developers/Tutorials/MigrationGuide/ObsoleteCodeRemoval"

From Slicer Wiki
Jump to: navigation, search
Tag: 2017 source edit
 
(19 intermediate revisions by 2 users not shown)
Line 175: Line 175:
 
Prior to supporting only C++11, <tt>0</tt> and [http://www.cplusplus.com/reference/cstring/NULL/ NULL] integral type were used to check for pointer value, and macro like <tt>Q_NULLPTR</tt> and <tt>ITK_NULLPTR</tt> macros were used to ensure usage of <tt>nullptr</tt> when C++11 support was enabled.
 
Prior to supporting only C++11, <tt>0</tt> and [http://www.cplusplus.com/reference/cstring/NULL/ NULL] integral type were used to check for pointer value, and macro like <tt>Q_NULLPTR</tt> and <tt>ITK_NULLPTR</tt> macros were used to ensure usage of <tt>nullptr</tt> when C++11 support was enabled.
  
Updating the code base to use <tt>nullptr</tt> is a 2-step process:
+
Updating the code base to use <tt>nullptr</tt> is done applying these methods:
* use clang-tidy for updating source code being explicitly compiled in the project.
+
* clang-tidy for updating source code being explicitly compiled in the project.
* directly update source code not being compiled (e.g conditionally compiled code, templated headers, comments)
+
* regular expression for replacing occurrences of <tt>NULL</tt> in source code not being compiled (e.g conditionally compiled code, templated headers, comments)
 +
* replacement of occurences of <tt>VTK_NULLPTR</tt>, <tt>ITK_NULLPTR</tt>, <tt>Q_NULLPTR</tt> with <tt>nullptr</tt>
  
'''Automatic update: Using clang-tidy'''
+
'''Using clang-tidy'''
  
 
This will iteratively check files for updates. It takes a while. The sources are only modified at the end of the process, so you could cancel anytime before with no consequences.
 
This will iteratively check files for updates. It takes a while. The sources are only modified at the end of the process, so you could cancel anytime before with no consequences.
Line 187: Line 188:
 
[http://clang.llvm.org/extra/clang-tidy/ clang-tidy] is a tool from clang. In Archlinux: sudo pacman -S clang, in other distros might be clang-extra-tools or similar.
 
[http://clang.llvm.org/extra/clang-tidy/ clang-tidy] is a tool from clang. In Archlinux: sudo pacman -S clang, in other distros might be clang-extra-tools or similar.
  
clang-tidy will substitute NULL and 0 for nullptr when appropiate. It is an iterative process, so it may take time. Also, it replaces all the compiled code, including external dependencies.
+
clang-tidy will substitute NULL and 0 for nullptr when appropriate. It is an iterative process, so it may take time. Also, it replaces all the compiled code, including external dependencies.
 
It is recommended to do a fresh build with the modified code, for testing it without modifications in any external repo.
 
It is recommended to do a fresh build with the modified code, for testing it without modifications in any external repo.
  
'''Direct update: Using shell script'''
+
'''Using regex for replacing NULL in comments and not-compiled code'''
  
 
After clang-tidy has modified the compiled code with the help of compile_commands.json, there will still be ignored instances of NULL and 0 in some headers, and comments.
 
After clang-tidy has modified the compiled code with the help of compile_commands.json, there will still be ignored instances of NULL and 0 in some headers, and comments.
Line 199: Line 200:
 
front of it. And there is not a quote after.
 
front of it. And there is not a quote after.
  
   ack --cpp "NULL" -l --print0 | xargs -0 -n 1 sed -i 's/\([^\"\_]\)NULL\([^\"\_]\)/\1nullptr\2/g'
+
   ack --type-add=cpp:ext:hxx,hpp,txx,tpp,cxx,cpp,c,cc --cpp "NULL" -l --print0 | xargs -0 -n 1 sed -i 's/\([^\"\_]\)NULL\([^\"\_]\)/\1nullptr\2/g'
  
 
2) The same than before but match end of line NULL
 
2) The same than before but match end of line NULL
   ack --cpp "NULL" -l --print0 | xargs -0 -n 1 sed -i 's/\([^\"\_]\)NULL$/\1nullptr/g'
+
   ack --type-add=cpp:ext:hxx,hpp,txx,tpp,cxx,cpp,c,cc --cpp "NULL" -l --print0 | xargs -0 -n 1 sed -i 's/\([^\"\_]\)NULL$/\1nullptr/g'
 +
 
 +
 
  
 
'''Replace ITK_NULLPTR, VTK_NULLPTR, Q_NULLPTR'''
 
'''Replace ITK_NULLPTR, VTK_NULLPTR, Q_NULLPTR'''
 +
 +
<!-- For an example of commit message, see [http://viewvc.slicer.org/viewvc.cgi/Slicer4?view=revision&revision=28022 r28022] -->
  
 
It was adapted from the ITK script in [https://github.com/InsightSoftwareConsortium/ITK/blob/v5.0rc01/Utilities/ITKv5Preparation/ReplaceITK_NULLPTRMacroNames.sh ReplaceITK_NULLPTRMacroNames.sh]
 
It was adapted from the ITK script in [https://github.com/InsightSoftwareConsortium/ITK/blob/v5.0rc01/Utilities/ITKv5Preparation/ReplaceITK_NULLPTRMacroNames.sh ReplaceITK_NULLPTRMacroNames.sh]
 +
 +
* <tt>ITK_NULLPTR</tt>:
  
 
<pre>git grep -l "ITK_NULLPTR" | fgrep -v CMakeLists.txt |fgrep -v .cmake | xargs sed -i '' -e "s/ITK_NULLPTR/nullptr/g"</pre>
 
<pre>git grep -l "ITK_NULLPTR" | fgrep -v CMakeLists.txt |fgrep -v .cmake | xargs sed -i '' -e "s/ITK_NULLPTR/nullptr/g"</pre>
  
<pre>git grep -l "ITK_NULLPTR" | fgrep -v CMakeLists.txt |fgrep -v .cmake | xargs sed -i '' -e "s/VTK_NULLPTR/nullptr/g"</pre>
+
* <tt>VTK_NULLPTR</tt>:
  
<pre>git grep -l "ITK_NULLPTR" | fgrep -v CMakeLists.txt |fgrep -v .cmake | xargs sed -i '' -e "s/Q_NULLPTR/nullptr/g"</pre>
+
<pre>git grep -l "VTK_NULLPTR" | fgrep -v CMakeLists.txt |fgrep -v .cmake | xargs sed -i '' -e "s/VTK_NULLPTR/nullptr/g"</pre>
 +
 
 +
* <tt>Q_NULLPTR</tt>:
 +
 
 +
<pre>git grep -l "Q_NULLPTR" | fgrep -v CMakeLists.txt |fgrep -v .cmake | xargs sed -i '' -e "s/Q_NULLPTR/nullptr/g"</pre>
  
 
===C++11: Update source code to use override===
 
===C++11: Update source code to use override===
Line 231: Line 242:
 
First we replace ITK_OVERRIDE and VTK_OVERRIDE for override:
 
First we replace ITK_OVERRIDE and VTK_OVERRIDE for override:
  
'''Replace ITK_OVERRIDE'''
+
'''Replace ITK_OVERRIDE and VTK_OVERRIDE'''
  
The regexp involving ITK were extracted from the [https://github.com/InsightSoftwareConsortium/ITK/blob/master/Utilities/ITKv5Preparation/ReplaceITK_OVERRIDEMacroNames.sh ReplaceITK_OVERRIDEMacroNames.sh] script in ITK.
+
For an example of commit message, see [http://viewvc.slicer.org/viewvc.cgi/Slicer4?view=revision&revision=28022 r28022]
  
  git grep -l "ITK_OVERRIDE" | fgrep -v CMakeLists.txt |fgrep -v .cmake | xargs sed -i '' -e "s/ITK_OVERRIDE/override/g"
+
The regexp involving ITK was extracted from the [https://github.com/InsightSoftwareConsortium/ITK/blob/master/Utilities/ITKv5Preparation/ReplaceITK_OVERRIDEMacroNames.sh ReplaceITK_OVERRIDEMacroNames.sh] script in ITK.
  
'''Replace VTK_OVERRIDE'''
+
* <tt>ITK_OVERRIDE</tt>
  
  git grep -l "VTK_OVERRIDE" | fgrep -v CMakeLists.txt |fgrep -v .cmake | xargs sed -i '' -e "s/VTK_OVERRIDE/override/g"
+
<pre>git grep -l "ITK_OVERRIDE" | fgrep -v CMakeLists.txt |fgrep -v .cmake | xargs sed -i '' -e "s/ITK_OVERRIDE/override/g"</pre>
 +
 
 +
* <tt>VTK_OVERRIDE</tt>
 +
 
 +
<pre>git grep -l "VTK_OVERRIDE" | fgrep -v CMakeLists.txt |fgrep -v .cmake | xargs sed -i '' -e "s/VTK_OVERRIDE/override/g"</pre>
  
 
'''Automatic update: Using clang-tidy'''
 
'''Automatic update: Using clang-tidy'''
 +
 +
'''Note''': Running ''clang-tidy -fix'' in parallel might cause problems, like appending multiple keywords to same function (override).  It can be fixed with a grep to remove duplicates.
  
 
This will iteratively check files for updates. It takes a while. The sources are only modified at the end of the process, so you could cancel anytime before with no consequences.
 
This will iteratively check files for updates. It takes a while. The sources are only modified at the end of the process, so you could cancel anytime before with no consequences.
Line 268: Line 285:
 
   sed: no input files
 
   sed: no input files
  
'''WIP:Adding missing overrides'''
+
===C++11: Update source code to use = delete ===
 +
 
 +
Instead of:
 +
  vtkImageRectangularSource(const vtkImageRectangularSource&);  /// Not implemented.
 +
  void operator=(const vtkImageRectangularSource&);  /// Not implemented.
 +
 
 +
Replace for:
 +
  vtkImageRectangularSource(const vtkImageRectangularSource&) = delete;
 +
  void operator=(const vtkImageRectangularSource&) = delete;
 +
 
 +
Same approach than before, use the compile_commands.json and clang-tidy in Slicer-build.
 +
 
 +
  run-clang-tidy.py -checks=-*,modernize-use-equals-delete -header-filter=.* -fix
 +
 
 +
Because we run it in parallel, multiple occurrences of = delete might have been added. Fix them with:
 +
 
 +
for n in `seq 1 15`;
 +
do
 +
  ack "= delete = delete" -l --print0 | xargs -0 -n 1 sed -i \
 +
  's/= delete = delete/= delete/g'
 +
done
 +
 
 +
And also delete comment /// Not implemented, as now it is clear with = delete.
 +
 
 +
  ack "delete" -l --print0 | xargs -0 -n 1 sed -E -i 's@delete;\s*\/*\s*[nN]ot [iI]mplemented\.?@delete;@g'
 +
  ack "delete" -l --print0 | xargs -0 -n 1 sed -E -i 's@delete;\s*\/*\s*purposely [nN]ot [iI]mplemented\.?@delete;@g'
 +
  ack "delete" -l --print0 | xargs -0 -n 1 sed -E -i 's@delete ITK\_DELETED\_FUNCTION;@delete;@g'
 +
 
 +
Next step in this area would be to move these declarations to the public interface (currently they are private).
 +
 
 +
<b>References:</b>
 +
 
 +
https://clang.llvm.org/extra/clang-tidy/checks/modernize-use-equals-delete.html
 +
 
 +
===C++11: Update source code to use = default ===
 +
 
 +
Instead of:
 +
  virtual ~DataRequest(){}
 +
 
 +
Replace for:
 +
  virtual ~DataRequest() = default;
 +
 
 +
Same approach than before, use the compile_commands.json and clang-tidy in Slicer-build.
 +
 
 +
  run-clang-tidy.py -checks=-*,modernize-use-equals-default -header-filter=.* -fix
 +
 
 +
Because we run it in parallel, multiple colons ;; might have been added. Fix them with:
 +
 
 +
for n in `seq 1 15`;
 +
do
 +
  ack "default" -l --print0 | xargs -0 -n 1 sed -i 's/= default;;/= default;/g'  
 +
done
 +
 
 +
Add an empty space before = default if there is none:
 +
  ack "default" -l --print0 | xargs -0 -n 1 sed -E -i 's/([^\s])= default;/\1 = default;/g'
 +
 
 +
<b>References:</b>
 +
 
 +
https://clang.llvm.org/extra/clang-tidy/checks/modernize-use-equals-default.html
 +
 
 +
===C++11: Use default member initialization ===
 +
 
 +
Instead of:
 +
 
 +
<pre>
 +
// vtkMRMLFooNode.h
 +
class VTK_MRML_EXPORT vtkMRMLFooNode : public vtkMRMLAbstractBarNode
 +
{
 +
  [...]
 +
 
 +
  bool DoSomething;
 +
  int InteractionMode;
 +
}
 +
 
 +
// vtkMRMLFooNode.cxx
 +
vtkMRMLFooNode::vtkMRMLFooNode()
 +
: DoSomething(true)
 +
, FooMode(FooModeAwesome)
 +
{
 +
[...]
 +
}
 +
</pre>
 +
 
 +
Replace with:
 +
 
 +
<pre>
 +
// vtkMRMLFooNode.h
 +
class VTK_MRML_EXPORT vtkMRMLFooNode : public vtkMRMLAbstractBarNode
 +
{
 +
  [...]
 +
 
 +
  bool DoSomething{true};
 +
  int FooMode{FooModeAwesome};
 +
};
 +
 
 +
// vtkMRMLFooNode.cxx
 +
vtkMRMLFooNode::vtkMRMLFooNode()
 +
{
 +
[...]
 +
}
 +
 
 +
// or this if constructor is empty after removing old-style default initialization
 +
 
 +
// vtkMRMLFooNode.cxx
 +
vtkMRMLFooNode::vtkMRMLFooNode() = default;
 +
</pre>
 +
 
  
We use the clang compiler to generate warnings about missing overrides with the option -Winconsistent-missing-override.
+
Same approach than before, use the compile_commands.json and clang-tidy in Slicer-build.
  
We then add the missing overrides with the script [https://github.com/Kitware/VTK/blob/master/Utilities/Maintenance/AddOverrides.py AddOverrides.py]
+
  run-clang-tidy.py -checks=-*,modernize-use-default-member-init -header-filter=.* -fix
  
  ninja clean
 
  ninja &> output.txt
 
  grep "overrides a member function but" output.txt | sort | uniq > overrides.txt
 
  python AddOverrides.py <overrides.txt>
 
  
 +
<b>References:</b>
  
However only one override was located, in the macro: qSlicerGetTittleMacro in qSlicerAbstractCoreModule.h
+
https://clang.llvm.org/extra/clang-tidy/checks/modernize-use-default-member-init.html
Note that the script `erroneously` adds the override to every instance of the macro.
 
The macro was updated.
 
 
 
  qSlicerGetTitleMacro("Loadable Hello world") override;
 

Latest revision as of 21:52, 15 April 2020

Home < Documentation < Nightly < Developers < Tutorials < MigrationGuide < ObsoleteCodeRemoval

Obsolete Code Removal

This section documents suggested code changes after removing support for a particular features. Each category has a short description, code snippets, a suggested upgrade path, and references to relevant commits.

Qt>=5.0.0: Remove obsolete code supporting Qt4 plugin infrastructure (C++)

These changes apply to Qt loadable modules and designer plugins

qSlicerNAMEModuleWidgetsPlugin.h

Before:

#include "vtkSlicerConfigure.h" // For Slicer_HAVE_QT5

// Qt includes
#ifdef Slicer_HAVE_QT5
#include <QtUiPlugin/QDesignerCustomWidgetCollectionInterface>
#else
#include <QDesignerCustomWidgetCollectionInterface>
#endif

[...]

class Q_SLICER_MODULE_SUBJECTHIERARCHY_WIDGETS_PLUGINS_EXPORT qSlicerSubjectHierarchyModuleWidgetsPlugin
  : public QObject
  , public QDesignerCustomWidgetCollectionInterface
{
  Q_OBJECT
#ifdef Slicer_HAVE_QT5
  Q_PLUGIN_METADATA(IID "org.qt-project.Qt.QDesignerCustomWidgetCollectionInterface")
#endif
  Q_INTERFACES(QDesignerCustomWidgetCollectionInterface);

After:

// Qt includes
#include <QtUiPlugin/QDesignerCustomWidgetCollectionInterface>

[...]

class Q_SLICER_MODULE_SUBJECTHIERARCHY_WIDGETS_PLUGINS_EXPORT qSlicerSubjectHierarchyModuleWidgetsPlugin
  : public QObject
  , public QDesignerCustomWidgetCollectionInterface
{
  Q_OBJECT
  Q_PLUGIN_METADATA(IID "org.qt-project.Qt.QDesignerCustomWidgetCollectionInterface")
  Q_INTERFACES(QDesignerCustomWidgetCollectionInterface);

qSlicerNAMEModule.h

Before:

class Q_SLICER_QTMODULES_SUBJECTHIERARCHY_EXPORT qSlicerSubjectHierarchyModule :
  public qSlicerLoadableModule
{
  Q_OBJECT
#ifdef Slicer_HAVE_QT5
  Q_PLUGIN_METADATA(IID "org.slicer.modules.loadable.qSlicerLoadableModule/1.0");
#endif

After:

class Q_SLICER_QTMODULES_SUBJECTHIERARCHY_EXPORT qSlicerSubjectHierarchyModule :
  public qSlicerLoadableModule
{
  Q_OBJECT
  Q_PLUGIN_METADATA(IID "org.slicer.modules.loadable.qSlicerLoadableModule/1.0");

qSlicerNAMEModuleWidgetsAbstractPlugin.h

Before:

  #include <QtGlobal>
  #if(QT_VERSION < QT_VERSION_CHECKS(5, 0, 0))
  #include <QDesignerCustomWidgetInterface>
  #else
  #include <QtUiPlugin/QDesignerCustomWidgetInterface>
  #endif

  [...]

  class Q_SLICER_MODULE_SEGMENTATIONS_WIDGETS_PLUGINS_EXPORT qSlicerSegmentationsModuleWidgetsAbstractPlugin
      : public QDesignerCustomWidgetInterface
  {
  #if (QT_VERSION >= QT_VERSION_CHECK(5, 0, 0))
    Q_PLUGIN_METADATA(IID "org.qt-project.Qt.QDesignerCustomWidgetInterface")
  #endif
    Q_INTERFACES(QDesignerCustomWidgetInterface);
  }

After:

  #include <QtUiPlugin/QDesignerCustomWidgetInterface>

  [...]

  class Q_SLICER_MODULE_SEGMENTATIONS_WIDGETS_PLUGINS_EXPORT qSlicerSegmentationsModuleWidgetsAbstractPlugin
      : public QDesignerCustomWidgetInterface
  {
    Q_PLUGIN_METADATA(IID "org.qt-project.Qt.QDesignerCustomWidgetInterface")
    Q_INTERFACES(QDesignerCustomWidgetInterface);
  }


qSlicerNAMEModule.cxx and qSlicerNAMEModuleWidgetsAbstractPlugin.cxx

Remove:

 #if (QT_VERSION < QT_VERSION_CHECK(5, 0, 0))
 #include <QtPlugin>
 Q_EXPORT_PLUGIN2(customwidgetplugin, qSlicerSegmentationsModuleWidgetsPlugin);
 #endif

Qt>=5.0.0: Simpler use of QHeaderView::setSectionResizeMode

This migration guide entry is obsolete: Documentation/Nightly/Developers/Tutorials/MigrationGuide#Qt5:_Fix_error:_.E2.80.98class_QHeaderView.E2.80.99_has_no_member_named_.E2.80.98setResizeMode.E2.80.99

Solution for Cpp

Before:

 #if (QT_VERSION < QT_VERSION_CHECK(5, 0, 0))
   this->SceneViewTableWidget->horizontalHeader()->setResizeMode(QHeaderView::Stretch);
   this->SceneViewTableWidget->horizontalHeader()->setResizeMode(QHeaderView::ResizeToContents);
   this->SceneViewTableWidget->horizontalHeader()->setResizeMode(SCENE_VIEW_DESCRIPTION_COLUMN, QHeaderView::Stretch);
 #else
   this->SceneViewTableWidget->horizontalHeader()->setSectionResizeMode(QHeaderView::Stretch);
   this->SceneViewTableWidget->horizontalHeader()->setSectionResizeMode(QHeaderView::ResizeToContents);
   this->SceneViewTableWidget->horizontalHeader()->setSectionResizeMode(SCENE_VIEW_DESCRIPTION_COLUMN, QHeaderView::Stretch);
 #endif

After:

 this->SceneViewTableWidget->horizontalHeader()->setSectionResizeMode(QHeaderView::Stretch);
 this->SceneViewTableWidget->horizontalHeader()->setSectionResizeMode(QHeaderView::ResizeToContents);
 this->SceneViewTableWidget->horizontalHeader()->setSectionResizeMode(SCENE_VIEW_DESCRIPTION_COLUMN, QHeaderView::Stretch);


Solution for Python

Before:

  def _setSectionResizeMode(header, *args, **kwargs):
    if version.parse(qt.Qt.qVersion()) < version.parse("5.0.0"):
      header.setResizeMode(*args, **kwargs)
    else:
      header.setSectionResizeMode(*args, **kwargs)

    [...]
   
  _setSectionResizeMode(self.horizontalHeader(), 0, qt.QHeaderView.Stretch)

After:

 self.horizontalHeader().setSectionResizeMode(0, qt.QHeaderView.Stretch)

C++11: Update source code to use nullptr

C++11 introduced the keyword nullptr allowing to explicitly identify a pointer of type std::nullptr_t. It avoids ambiguous function overloads. See also https://stackoverflow.com/questions/20509734/null-vs-nullptr-why-was-it-replaced

Prior to supporting only C++11, 0 and NULL integral type were used to check for pointer value, and macro like Q_NULLPTR and ITK_NULLPTR macros were used to ensure usage of nullptr when C++11 support was enabled.

Updating the code base to use nullptr is done applying these methods:

  • clang-tidy for updating source code being explicitly compiled in the project.
  • regular expression for replacing occurrences of NULL in source code not being compiled (e.g conditionally compiled code, templated headers, comments)
  • replacement of occurences of VTK_NULLPTR, ITK_NULLPTR, Q_NULLPTR with nullptr

Using clang-tidy

This will iteratively check files for updates. It takes a while. The sources are only modified at the end of the process, so you could cancel anytime before with no consequences.

  1. Compile Slicer with the CMake option -DCMAKE_EXPORT_COMPILE_COMMANDS:BOOL=ON to generate the file compile_commands.json at configuration time.
  2. Run clang-tidy on the generated compile_commands.json (in the build folder: Slicer-build)
 run-clang-tidy.py -header-filter='.*' -checks='-*,modernize-use-nullptr' -fix

clang-tidy is a tool from clang. In Archlinux: sudo pacman -S clang, in other distros might be clang-extra-tools or similar.

clang-tidy will substitute NULL and 0 for nullptr when appropriate. It is an iterative process, so it may take time. Also, it replaces all the compiled code, including external dependencies. It is recommended to do a fresh build with the modified code, for testing it without modifications in any external repo.

Using regex for replacing NULL in comments and not-compiled code

After clang-tidy has modified the compiled code with the help of compile_commands.json, there will still be ignored instances of NULL and 0 in some headers, and comments.

Manually replace with the following regex to change those.

1) Change NULL to nullptr except when there is a quote or an underscore in front of it. And there is not a quote after.

 ack --type-add=cpp:ext:hxx,hpp,txx,tpp,cxx,cpp,c,cc --cpp "NULL" -l --print0 | xargs -0 -n 1 sed -i 's/\([^\"\_]\)NULL\([^\"\_]\)/\1nullptr\2/g'

2) The same than before but match end of line NULL

 ack --type-add=cpp:ext:hxx,hpp,txx,tpp,cxx,cpp,c,cc --cpp "NULL" -l --print0 | xargs -0 -n 1 sed -i 's/\([^\"\_]\)NULL$/\1nullptr/g'


Replace ITK_NULLPTR, VTK_NULLPTR, Q_NULLPTR


It was adapted from the ITK script in ReplaceITK_NULLPTRMacroNames.sh

  • ITK_NULLPTR:
git grep -l "ITK_NULLPTR" | fgrep -v CMakeLists.txt |fgrep -v .cmake | xargs sed -i '' -e "s/ITK_NULLPTR/nullptr/g"
  • VTK_NULLPTR:
git grep -l "VTK_NULLPTR" | fgrep -v CMakeLists.txt |fgrep -v .cmake | xargs sed -i '' -e "s/VTK_NULLPTR/nullptr/g"
  • Q_NULLPTR:
git grep -l "Q_NULLPTR" | fgrep -v CMakeLists.txt |fgrep -v .cmake | xargs sed -i '' -e "s/Q_NULLPTR/nullptr/g"

C++11: Update source code to use override

From: https://clang.llvm.org/extra/clang-tidy/checks/modernize-use-override.html

Adds override (introduced in C++11) to overridden virtual functions and removes virtual from those functions as it is not required.

virtual on non base class implementations was used to help indicate to the user that a function was virtual. C++ compilers did not use the presence of this to signify an overriden function.

In C++ 11 override and final keywords were introduced to allow overridden functions to be marked appropriately. Their presence allows compilers to verify that an overridden function correctly overrides a base class implementation.

This can be useful as compilers can generate a compile time error when:

  1. The base class implementation function signature changes.
  2. The user has not created the override with the correct signature.

First we replace ITK_OVERRIDE and VTK_OVERRIDE for override:

Replace ITK_OVERRIDE and VTK_OVERRIDE

For an example of commit message, see r28022

The regexp involving ITK was extracted from the ReplaceITK_OVERRIDEMacroNames.sh script in ITK.

  • ITK_OVERRIDE
git grep -l "ITK_OVERRIDE" | fgrep -v CMakeLists.txt |fgrep -v .cmake | xargs sed -i '' -e "s/ITK_OVERRIDE/override/g"
  • VTK_OVERRIDE
git grep -l "VTK_OVERRIDE" | fgrep -v CMakeLists.txt |fgrep -v .cmake | xargs sed -i '' -e "s/VTK_OVERRIDE/override/g"

Automatic update: Using clang-tidy

Note: Running clang-tidy -fix in parallel might cause problems, like appending multiple keywords to same function (override). It can be fixed with a grep to remove duplicates.

This will iteratively check files for updates. It takes a while. The sources are only modified at the end of the process, so you could cancel anytime before with no consequences.

Compile Slicer with the CMake option -DCMAKE_EXPORT_COMPILE_COMMANDS:BOOL=ON to generate the file compile_commands.json at configuration time. Run clang-tidy on the generated compile_commands.json (in the build folder: Slicer-build)

 run-clang-tidy.py -checks=-*,modernize-use-override  -header-filter=.* -fix

Manual fix

Multiple override are added to the same method. For example:

 void PrintSelf(ostream& os, vtkIndent indent) override override override;

We clean them with (15 here is just a high enough number):

 for n in `seq 1 15`;
 do
     ack "override override" -l --print0 | xargs -0 -n 1 sed -i \
     's/override override/override/g'
 done

When all of them are fixed, you should see in the terminal:

 sed: no input files

C++11: Update source code to use = delete

Instead of:

 vtkImageRectangularSource(const vtkImageRectangularSource&);  /// Not implemented.
 void operator=(const vtkImageRectangularSource&);  /// Not implemented.

Replace for:

 vtkImageRectangularSource(const vtkImageRectangularSource&) = delete;
 void operator=(const vtkImageRectangularSource&) = delete;

Same approach than before, use the compile_commands.json and clang-tidy in Slicer-build.

 run-clang-tidy.py -checks=-*,modernize-use-equals-delete -header-filter=.* -fix

Because we run it in parallel, multiple occurrences of = delete might have been added. Fix them with:

for n in `seq 1 15`;
do
  ack "= delete = delete" -l --print0 | xargs -0 -n 1 sed -i \
  's/= delete = delete/= delete/g'
done

And also delete comment /// Not implemented, as now it is clear with = delete.

 ack "delete" -l --print0 | xargs -0 -n 1 sed -E -i 's@delete;\s*\/*\s*[nN]ot [iI]mplemented\.?@delete;@g'
 ack "delete" -l --print0 | xargs -0 -n 1 sed -E -i 's@delete;\s*\/*\s*purposely [nN]ot [iI]mplemented\.?@delete;@g'
 ack "delete" -l --print0 | xargs -0 -n 1 sed -E -i 's@delete ITK\_DELETED\_FUNCTION;@delete;@g'

Next step in this area would be to move these declarations to the public interface (currently they are private).

References:

https://clang.llvm.org/extra/clang-tidy/checks/modernize-use-equals-delete.html

C++11: Update source code to use = default

Instead of:

 virtual ~DataRequest(){}

Replace for:

 virtual ~DataRequest() = default;

Same approach than before, use the compile_commands.json and clang-tidy in Slicer-build.

 run-clang-tidy.py -checks=-*,modernize-use-equals-default -header-filter=.* -fix

Because we run it in parallel, multiple colons ;; might have been added. Fix them with:

for n in `seq 1 15`;
do
  ack "default" -l --print0 | xargs -0 -n 1 sed -i 's/= default;;/= default;/g' 
done

Add an empty space before = default if there is none:

 ack "default" -l --print0 | xargs -0 -n 1 sed -E -i 's/([^\s])= default;/\1 = default;/g'

References:

https://clang.llvm.org/extra/clang-tidy/checks/modernize-use-equals-default.html

C++11: Use default member initialization

Instead of:

// vtkMRMLFooNode.h
class VTK_MRML_EXPORT vtkMRMLFooNode : public vtkMRMLAbstractBarNode
{
  [...]

  bool DoSomething; 
  int InteractionMode;
}

// vtkMRMLFooNode.cxx
vtkMRMLFooNode::vtkMRMLFooNode()
: DoSomething(true)
, FooMode(FooModeAwesome)
 {
 [...]
 }

Replace with:

// vtkMRMLFooNode.h
class VTK_MRML_EXPORT vtkMRMLFooNode : public vtkMRMLAbstractBarNode
{
  [...]

  bool DoSomething{true}; 
  int FooMode{FooModeAwesome};
 };

// vtkMRMLFooNode.cxx
vtkMRMLFooNode::vtkMRMLFooNode()
 {
 [...]
 }

// or this if constructor is empty after removing old-style default initialization

// vtkMRMLFooNode.cxx
vtkMRMLFooNode::vtkMRMLFooNode() = default;


Same approach than before, use the compile_commands.json and clang-tidy in Slicer-build.

 run-clang-tidy.py -checks=-*,modernize-use-default-member-init -header-filter=.* -fix


References:

https://clang.llvm.org/extra/clang-tidy/checks/modernize-use-default-member-init.html