Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENH: Providing ITKv5 updates for C++11 #23

Merged
merged 5 commits into from
Jan 22, 2018
Merged

ENH: Providing ITKv5 updates for C++11 #23

merged 5 commits into from
Jan 22, 2018

Conversation

hjmjohnson
Copy link
Contributor

This pull request incorporates code changes
that happened in parallel with updating the
core ITK code to ITKv5.

Provide remove virtual and override

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,

virtual for the base class function declaration.
This is technically necessary.

Use override (only) for a derived class' override.
This helps with maintenance.
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"
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.

SRCDIR= #My local SRC
BLDDIR= #My local BLD

clang-tidy -p ${BLD}  $(find Modules/[A-J]*  -name "*.cxx" \|fgrep -v ThirdParty) -checks=-*,modernize-use-nullptr  -header-filter=.* -fix
clang-tidy -p ${BLD}  $(find Modules/[K-Z]*  -name "*.cxx" \|fgrep -v ThirdParty) -checks=-*,modernize-use-nullptr  -header-filter=.* -fix
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.
@hjmjohnson
Copy link
Contributor Author

@thewtex Matt The circle CI needs updating for several of the remote modules. Should we ignore this for now, or do you have time to fix it?

  • cmake -G Ninja -DITK_DIR:PATH=/usr/src/ITK-build -DCMAKE_BUILD_TYPE:STRING=Release -DBUILDNAME:STRING=External-RLEImage-ITKv5-2018-01-22_14_26_39-2f3fa6f /usr/src/ITKRLEImage
    CMake Error at CMakeLists.txt:1 (cmake_minimum_required):
    CMake 3.9.5 or higher is required. You are running version 3.6.0

@thewtex
Copy link
Contributor

thewtex commented Jan 22, 2018

We should have ITK working well before putting time into updating the CI builds.

@hjmjohnson
Copy link
Contributor Author

@thewtex Agreed. so ignoring for now.

@hjmjohnson hjmjohnson merged commit 520cc0f into master Jan 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants