WIP: ITK initial conversion 20171210 by hjmjohnson · Pull Request #36 · InsightSoftwareConsortium/ITK

hjmjohnson · 2017-12-13T13:37:02Z

This pull request provides a starting point for ITKv5 development. This development branch will not be managed on the gerrit review system.

This set of patches is NOT intended for merging. Several iterations are anticipated.

dzenanz

"Require CMake 3.5.2" in commit message is misleading, as the commit requires version 3.8.2.

The rest looks like a good start!

dzenanz · 2017-12-13T17:36:19Z

CMakeLists.txt

+  set(CMAKE_CXX_STANDARD_REQUIRED ON)
+endif()
+if(NOT CMAKE_CXX_EXTENSIONS)
+  set(CMAKE_CXX_EXTENSIONS ON)


Why explicitly enable extensions? Wouldn't it be easier to write cross-platform code with compiler extensions disabled?

blowekamp · 2017-12-15T16:47:32Z

CMakeLists.txt

+#####
+##  Set the default target properties for ITK
+if(NOT CMAKE_CXX_STANDARD)
+  set(CMAKE_CXX_STANDARD 11) # Supported values are ``11``, ``14``, and ``17``.


Should these be made cache variables so that the user can set them and knows they are set?

dzenanz

Great job Hans, keep it up!

dzenanz · 2017-12-18T16:36:28Z

Utilities/ITKv5Preparation/ReplaceitkGetObjectMacro.sh

+  fgrep -v itk_compiler_detection.h | fgrep -v CMakeLists.txt |fgrep -v .cmake | \
+  xargs sed -i '' -e \"s/${oldstring}/${newstring}/g\"
+
+For a full description of the rational for this change


rationale? (missing 'e')

dzenanz · 2017-12-18T17:31:32Z

Modules/Remote/MorphologicalContourInterpolation.remote.cmake

 'Continued development and maintenance of the ITK-SNAP 3D image segmentation software'."
  GIT_REPOSITORY ${git_protocol}://github.com/KitwareMedical/ITKMorphologicalContourInterpolation.git
-  GIT_TAG 608853faf597c38132eaff8586f1f9c5c31f70aa
+  GIT_TAG ITKv5


And until these tags are added, enabling remote modules will cause errors? That's certain to draw attention 😄

These are not intended for long term use. I think they are being pushed now. I'll test on a secondary machine soon.

I meant, if anyone tries building your PR he will be clearly reminded that remote modules should not be turned on 😄

dzenanz · 2017-12-18T17:33:21Z

Examples/Filtering/CompositeFilterExample.cxx

 //  Software Guide : EndCodeSnippet

-  virtual void GenerateData() override;
+  void GenerateData() override;


It is great you are doing this for examples too, as examples will be copied many times.

blowekamp · 2017-12-22T16:45:08Z

Great job Has! here is the CircleCI build on CDash:
https://open.cdash.org/buildSummary.php?buildid=5192170

There is just one little warning.

blowekamp · 2017-12-22T16:49:51Z

I see the ITKv3 registration framework examples are being removed. There is not plans to remove the older v3 Registration framework?

dzenanz · 2018-01-03T22:54:33Z

Modules/Core/Transform/include/itkRigid3DTransform.h

  typedef SmartPointer<Self>                                    Pointer;
  typedef SmartPointer<const Self>                              ConstPointer;

+//HACK #ifdef ITKV3_COMPATIBILITY


Is this only temporary?

blowekamp · 2018-01-05T16:19:27Z

CMake/itkApple.cmake

+  CMAKE_OSX_SYSROOT)
+
+if (CMAKE_OSX_DEPLOYMENT_TARGET AND
+    CMAKE_OSX_DEPLOYMENT_TARGET VERSION_LESS "10.7")


OSX SDK 10.6 is the standard SDK for Python.

@blowekamp This was copied and pasted from the VTK v9 configuration infrasturcture.

https://github.com/Kitware/VTK/blob/master/CMake/vtkApple.cmake

I was just following their lead.

This is now a separate patch.

dzenanz · 2018-01-05T17:53:01Z

Examples/CMakeLists.txt

 add_subdirectory(Statistics)
 add_subdirectory(RegistrationITKv4)
-
-if(ITKV3_COMPATIBILITY)


I think we concluded that examples for v3 registration should remain. Just remove this if condition, and keep the examples.

dzenanz · 2018-01-05T17:56:45Z

Could you modify STYLE: Remove all vestiges of ITKv3 code to keep the examples and push all the commits up to COMP: Enforce building with C++11 to Gerrit?

ITK version 5 will require C++11 syntax when building. During initial implementation of C++11 rigorously enforce C++11 language support. Require CMake 3.8.2 at a minimum version. Change-Id: I391a64b66135911acb455c539a70a01ee2e5bbf9

With CMake 3.8.2 and greater the static backwards compatibly header files can be removed in favor of dynamically creating these header files from cmake using write_compiler_detection_header Change-Id: Ia74bed1c5047dba0621fad7b56fb2ad61812c7f8

circleci environment needs to include at leasst version 3.8.2 for cmake to support ITKv5. Change-Id: I55de27c74ad515fe3ca0a4412b5cc90aaa0e41ab

The following is now always true for C++11: ITK_COMPILED_CXX_STANDARD_VERSION >= 201103L Remove contingent code compilation. Change-Id: Iee87889676c8b17d75ae109acd8cf5faab107d5e

Use clang-tidy to add ITK_OVERRIDE, and to remove redundant virtual on functions. cd ../ITK; clang-tidy -p ITK-clangtidy $(find Modules/[A-J]* -name "*.cxx" |fgrep -v ThirdParty) -checks=-*,modernize-use-override -header-filter=.* -fix clang-tidy -p ITK-clangtidy $(find Modules/[K-Z]* -name "*.cxx" |fgrep -v ThirdParty) -checks=-*,modernize-use-override -header-filter=.* -fix https://stackoverflow.com/questions/39932391/virtual-override-or-both-c When you override a function you don't technically need to write either virtual or override. The original base class declaration needs the keyword virtual to mark it as virtual. In the derived class the function is virtual by way of having the ¹same type as the base class function. However, an override can help avoid bugs by producing a compilation error when the intended override isn't technically an override. E.g. that the function type isn't exactly like the base class function. Or that a maintenance of the base class changes that function's type, e.g. adding a defaulted argument. In the same way, a virtual keyword in the derived class can make such a bug more subtle, by ensuring that the function is still is virtual in further derived classes. So the general advice is, Use virtual for the base class function declaration. This is technically necessary. Use override (only) for a derived class' override. This helps with maintenance. Change-Id: Ic99248fbd081f264fab8fe9cf0ffc0a98bace639

git grep -l "ITK_OVERRIDE" | fgrep -v itk_compiler_detection.h | fgrep -v CMakeLists.txt |fgrep -v .cmake | xargs sed -i '' -e "s/ITK_OVERRIDE/override/g" Change-Id: I1147053472d97b2d4a353fddded7571ca6243368

git grep -l \"ITK_NULLPTR\" | \ fgrep -v itk_compiler_detection.h | fgrep -v CMakeLists.txt |fgrep -v .cmake | \ xargs sed -i '' -e \"s/ITK_NULLPTR/nullptr/g\"

The check converts the usage of null pointer constants (eg. NULL, 0) to use the new C++11 nullptr keyword. clang-tidy -p ITK-clangtidy $(find Modules/[A-J]* -name "*.cxx" |fgrep -v ThirdParty) -checks=-*,modernize-use-nullptr -header-filter=.* -fix clang-tidy -p ITK-clangtidy $(find Modules/[K-Z]* -name "*.cxx" |fgrep -v ThirdParty) -checks=-*,modernize-use-nullptr -header-filter=.* -fix Change-Id: Ia69b1993cdad5931a74d2bc30eafadae0460aa45

Pull over settings from VTK build so that similar configurations are used. Change-Id: I72832bf8183abc5b05bda17d30bc3d6fce027323

OSX SDK 10.6 is still the standard for python integration. Change-Id: I1bb8cdd9ccdb527a5148bdc6d55c5b18a223aaa2

Prefer to use nullptr keyword for indicating a null pointer. Change-Id: I56318fdedc7efcb6834e1e4f78dbeff127206757

Describe function overrides using the override keyword from C++11. run-clang-tidy.py -checks="-*,modernize-use-override" -header-filter=".*" -fix Change-Id: I25ed9c035c128ae22c6fbc06ad8dcbc6b518f1c7

Each threader may have a different type for the default initialization. Keep nullptr separate from the integer value of 0, and NULL. Change-Id: I3239afde505da25032bf8389ce5360195bfa79b5

In file included from ITK/Modules/Core/Mesh/src/itkMeshRegion.cxx:18: ITK/Modules/Core/Mesh/include/itkMeshRegion.h:81:32: warning: comparison 'itk::SizeValueType' (aka 'unsigned long') <= 18446744073709551615 is always true [-Wtautological-constant-compare] if ( num >= 1 ) Change-Id: I6753da58645346f56b9aea777dbbeb4a63cd8034

This change is intended to ease the burden for developers during the migration to ITKv5. Anticipating several changes needed to bring Remote modules into compliance with ITKv5. Change-Id: I8a3f2ab098d5e3a8f6b8d471075e64ccfc9b43de

Some headers from C library were deprecated in C++ and are no longer welcome in C++ codebases. Some have no effect in C++. For more details refer to the C++ 14 Standard [depr.c.headers] section. This patch replaces C standard library headers with their C++ alternatives and removes redundant ones. Change-Id: I76376c552e1d836f8909a5ea7230090e80e232ce

dzenanz reviewed Dec 13, 2017

View reviewed changes

blowekamp reviewed Dec 15, 2017

View reviewed changes

hjmjohnson force-pushed the ITKv5_InitialConversion_20171210 branch from dba4329 to efbe6af Compare December 18, 2017 03:23

dzenanz reviewed Dec 18, 2017

View reviewed changes

hjmjohnson force-pushed the ITKv5_InitialConversion_20171210 branch 4 times, most recently from 26d8fec to d1dc3f1 Compare December 22, 2017 12:20

hjmjohnson force-pushed the ITKv5_InitialConversion_20171210 branch 9 times, most recently from 40a821a to e09d721 Compare December 30, 2017 16:36

dzenanz reviewed Jan 3, 2018

View reviewed changes

blowekamp reviewed Jan 5, 2018

View reviewed changes

hjmjohnson force-pushed the ITKv5_InitialConversion_20171210 branch 2 times, most recently from 13316a1 to aa6ab2d Compare January 5, 2018 16:46

dzenanz reviewed Jan 5, 2018

View reviewed changes

hjmjohnson force-pushed the ITKv5_InitialConversion_20171210 branch 2 times, most recently from 12c2b7a to dbce04e Compare January 6, 2018 22:04

hjmjohnson added 3 commits January 8, 2018 13:35

COMP: Enforce building with C++11

9f43fd3

ITK version 5 will require C++11 syntax when building. During initial implementation of C++11 rigorously enforce C++11 language support. Require CMake 3.8.2 at a minimum version. Change-Id: I391a64b66135911acb455c539a70a01ee2e5bbf9

COMP: Change min cmake version to 3.8.2

a9248a5

circleci environment needs to include at leasst version 3.8.2 for cmake to support ITKv5. Change-Id: I55de27c74ad515fe3ca0a4412b5cc90aaa0e41ab

hjmjohnson added 13 commits January 8, 2018 13:35

STYLE: Remove conditional version 201103L code

cd615ce

The following is now always true for C++11: ITK_COMPILED_CXX_STANDARD_VERSION >= 201103L Remove contingent code compilation. Change-Id: Iee87889676c8b17d75ae109acd8cf5faab107d5e

COMP: Use C++11 override directly

e81213f

git grep -l "ITK_OVERRIDE" | fgrep -v itk_compiler_detection.h | fgrep -v CMakeLists.txt |fgrep -v .cmake | xargs sed -i '' -e "s/ITK_OVERRIDE/override/g" Change-Id: I1147053472d97b2d4a353fddded7571ca6243368

COMP: Use C++11 ITK_NULLPTR directly

1994c8e

git grep -l \"ITK_NULLPTR\" | \ fgrep -v itk_compiler_detection.h | fgrep -v CMakeLists.txt |fgrep -v .cmake | \ xargs sed -i '' -e \"s/ITK_NULLPTR/nullptr/g\"

COMP: Modularize cmake config like VTK.

ff26c4d

Pull over settings from VTK build so that similar configurations are used. Change-Id: I72832bf8183abc5b05bda17d30bc3d6fce027323

COMP: Support OSX Python default SDK builds

b71e88a

OSX SDK 10.6 is still the standard for python integration. Change-Id: I1bb8cdd9ccdb527a5148bdc6d55c5b18a223aaa2

STYLE: Prefer nullptr for C++11

09754d0

Prefer to use nullptr keyword for indicating a null pointer. Change-Id: I56318fdedc7efcb6834e1e4f78dbeff127206757

STYLE: Use override statements for C++11

70d871e

Describe function overrides using the override keyword from C++11. run-clang-tidy.py -checks="-*,modernize-use-override" -header-filter=".*" -fix Change-Id: I25ed9c035c128ae22c6fbc06ad8dcbc6b518f1c7

COMP: Need to match type for different threaders

52b06d3

Each threader may have a different type for the default initialization. Keep nullptr separate from the integer value of 0, and NULL. Change-Id: I3239afde505da25032bf8389ce5360195bfa79b5

COMP: Change all RemoteModules GIT_TAG to ITKv5

b1d5bd2

This change is intended to ease the burden for developers during the migration to ITKv5. Anticipating several changes needed to bring Remote modules into compliance with ITKv5. Change-Id: I8a3f2ab098d5e3a8f6b8d471075e64ccfc9b43de

hjmjohnson force-pushed the ITKv5_InitialConversion_20171210 branch from dbce04e to c00dfc5 Compare January 8, 2018 19:36

hjmjohnson closed this Jan 10, 2018

hjmjohnson mentioned this pull request Apr 11, 2020

BUG: Fix mattes metric with sse instructions #1762

Merged

2 tasks

Uh oh!

Conversation

hjmjohnson commented Dec 13, 2017

Uh oh!

dzenanz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dzenanz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blowekamp commented Dec 22, 2017

Uh oh!

blowekamp commented Dec 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dzenanz commented Jan 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants