ENH: Upgrade ITK from C++14 to C++17 by N-Dekker · Pull Request #3969 · InsightSoftwareConsortium/ITK

N-Dekker · 2023-03-17T22:22:53Z

Follow-up to pull request #2563 commit daec0fd "ENH: Upgrade ITK from C++11 to C++14"

Suggested by Dženan Zukić (@dzenanz) 😃 at #3933 (comment) Still to be discussed.

More information about C++17:

Discussion "Shall we start using C++17 at the master branch?" opened at https://discourse.itk.org/t/shall-we-start-using-c-17-at-the-master-branch/5828

N-Dekker · 2023-03-18T16:53:35Z

Still need to figure out how to upgrade the Python wrapping from C++14 to C++17! Any clue? Looking at Darwin-Build8553-PR3969-CXX_STANDARD-from-14-to-17-Python:

cd /.../s-build/Wrapping/Modules/ITKCommon && env SDKROOT=/Applications/Xcode_13.2.1.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk MACOSX_DEPLOYMENT_TARGET=11.7 /.../s-build/Wrapping/Generators/CastXML/castxml/bin/castxml -o /.../s-build/Wrapping/castxml_inputs/ITKCommonBase.xml --castxml-gccxml --castxml-start wrapping --castxml-cc-gnu "(" /Applications/Xcode_13.2.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ -Wall -Wcast-align -Wdisabled-optimization -Wextra -Wformat=2 -Winvalid-pch -Wno-format-nonliteral -Wpointer-arith -Wshadow -Wunused -Wwrite-strings -funit-at-a-time -Wno-strict-overflow -Wno-deprecated -Wno-invalid-offsetof -Wno-undefined-var-template -Woverloaded-virtual -std=c++14 ")" -w -c @/.../s-build/Wrapping/castxml_inputs/.castxml.inc /.../s-build/Wrapping/castxml_inputs/ITKCommonBase.cxx

It still says -std=c++14. That should be -std=c++17, right?

Similarly, Linux-Build8480-PR3969-CXX_STANDARD-from-14-to-17-Python says:

cd /.../s-build/Wrapping/Modules/ITKCommon && /.../s-build/Wrapping/Generators/CastXML/castxml/bin/castxml -o /.../s-build/Wrapping/castxml_inputs/ITKCommonBase.xml --castxml-gccxml --castxml-start wrapping --castxml-cc-gnu "(" /usr/bin/c++ -Wall -Wcast-align -Wdisabled-optimization -Wextra -Wformat=2 -Winvalid-pch -Wno-format-nonliteral -Wpointer-arith -Wshadow -Wunused -Wwrite-strings -funit-at-a-time -Wno-strict-overflow -Wno-deprecated -Wno-invalid-offsetof -Woverloaded-virtual -Wstrict-null-sentinel -msse2 -msse2 -std=c++14 ")" -w -c @/.../s-build/Wrapping/castxml_inputs/.castxml.inc /.../s-build/Wrapping/castxml_inputs/ITKCommonBase.cxx

Also still says -std=c++14! What to do?

issakomi · 2023-03-18T19:24:39Z

Also still says -std=c++14!

In Wrapping/macro_files/itk_auto_load_submodules.cmake, here

    set(_castxml_cc_flags ${CMAKE_CXX_FLAGS})
    if(CMAKE_CXX_EXTENSIONS)
        set(_castxml_cc_flags "${_castxml_cc_flags} ${CMAKE_CXX14_EXTENSION_COMPILE_OPTION}")
    else()
        set(_castxml_cc_flags "${_castxml_cc_flags} ${CMAKE_CXX14_STANDARD_COMPILE_OPTION}")
    endif()

N-Dekker · 2023-03-18T21:54:37Z

In Wrapping/macro_files/itk_auto_load_submodules.cmake, here

Thank you @issakomi ! That's very helpful 😃 ! Just force-pushed!

dzenanz · 2023-03-20T19:32:32Z

Maybe rebase on new master now that silence PR was merged?

Follow-up to pull request InsightSoftwareConsortium#2563 commit daec0fd "ENH: Upgrade ITK from C++11 to C++14"

Addresses Visual C++ MSVC 19.29.30148.0 (MSVC/14.29.30133) warnings, saying: > warning C4996: 'std::result_of_t': warning STL4014: std::result_of and > std::result_of_t are deprecated in C++17. They are superseded by > std::invoke_result and std::invoke_result_t. You can define > _SILENCE_CXX17_RESULT_OF_DEPRECATION_WARNING or > _SILENCE_ALL_CXX17_DEPRECATION_WARNINGS to acknowledge that you have received > this warning. Note that C++20 has even removed `std::result_of_t`!

Worked around a GCC bug which appeared when having compiler options "-Wextra" and "-std=c++17", that caused warning messages, like: > itkSizeGTest.cxx: In instantiation of 'Size_Fill_Test::TestBody()::<lambda(auto:23)>: > Modules/Core/Common/test/itkSizeGTest.cxx:126:29: required from here > itkSize.h:70:28: note: 'struct itk::Size<1>' has no user-provided default constructor > itkSize.h:229:40: note: and the implicitly-defined constructor does not initialize 'itk::Size<1>::SizeValueType itk::Size<1>::m_InternalArray [1]' At Azure.fv-az105-696, Linux-Build9859 (GNU 9.4.0): https://open.cdash.org/viewBuildError.php?type=1&buildid=8556515 This compiler bug (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91070) was fixed with GCC version 10.

dzenanz

Good progress!

Follow-up to pull request InsightSoftwareConsortium#2563 commit 4e812d6 "COMP: Require compiler versions that support C++14" Resources: C++ reference - Compiler support for C++17 https://en.cppreference.com/w/cpp/compiler_support/17 Note that for C++17 "hardware interference size" support, we would need GCC 12 and Clang 15. C++17 Support in GCC https://gcc.gnu.org/projects/cxx-status.html#cxx17 Announcing: MSVC Conforms to the C++ Standard, Ulzii Luvsanbat, May 7th, 2018 https://devblogs.microsoft.com/cppblog/announcing-msvc-conforms-to-the-c-standard/ C++17 Features Supported by Intel C++ Compiler Classic, Last Updated: 09/12/2020 By Anoop Madhusoodhanan Prabha https://www.intel.com/content/www/us/en/developer/articles/news/c17-features-supported-by-c-compiler.html Xcode clang version record, Masayuki Yamaya, December 14, 2022 https://gist.github.com/yamaya/2924292

Following ITK pull request InsightSoftwareConsortium/ITK#3969 "ENH: Upgrade ITK from C++14 to C++17" Follow-up to pull request #513 commit d98d9c0 "ENH: Upgrade elastix from C++11 to C++14".

kwrobot-v1 · 2023-03-29T16:43:02Z

Errors:

Failed to fetch from the repository: host error: no result after exponential backoff.

dzenanz · 2023-03-29T16:43:15Z

There seems to be consensus support in the forum discussion. Please review here too.

kwrobot-v1 · 2023-03-29T16:43:20Z

Errors:

Failed to fetch from the repository: host error: no result after exponential backoff.

dzenanz · 2023-03-29T16:44:18Z

@bradking kwrobot is misbehaving recently. Do you have any idea why? Or even better, a fix?

bradking · 2023-03-29T17:28:30Z

It's more fallout from the GitHub host key change. It should be fixed now.

seanm · 2023-03-31T15:57:30Z

Hmm, unfortunate that such a big change was made with such a short discussion period. I just discovered this last night.

Requiring Xcode 10 consequently drops support for macOS 10.10, 10.11, and 10.12 because Xcode 10 can only run on macOS 10.13.

Dropping 10.10 support is not so bad because 10.11 supports all the same Macs as 10.10. I'll retire the Rogue16 nightly cdash build consequently.

Dropping 10.11 support though means dropping support for various Mac models because not all Macs that can run 10.11 can run 10.12, as 10.12 dropped support for various models. The newest Xcode that can be run on 10.11 is Xcode 8.2.1, which is what Rogue7 runs.

Dropping 10.12 support is not so bad because 10.13 supports all the same Macs as 10.11.

I still have customers with older Macs (that are thus stuck on 10.11), and so it's been nice to build and run nightly ITK tests with my corresponding bots. I suppose I don't necessarily need to build on these old OSes though: hopefully an ITK built on a newer macOS/Xcode can still result in an executable that can still run on these older OSes. I imagine that will be less about C++ itself but more about what macOS 10.11's libc++ supports or doesn't. I'll update my 10.13 bot (Rogue22) to reduce the CMAKE_OSX_DEPLOYMENT_TARGET to 10.11 and let's see what that gives...

Following ITK pull request InsightSoftwareConsortium/ITK#3969 commit InsightSoftwareConsortium/ITK@513507e "ENH: Upgrade ITK from C++14 to C++17" Follow-up to pull request #513 commit d98d9c0 "ENH: Upgrade elastix from C++11 to C++14".

seanm · 2023-04-14T20:57:40Z

So the Rogue22 bot seems to be building fine. Please keep an eye on it when you start to use new C++17 features.

github-actions bot added type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Core Issues affecting the Core module labels Mar 17, 2023

dzenanz reviewed Mar 18, 2023

View reviewed changes

Comment thread CMakeLists.txt Outdated

N-Dekker force-pushed the CXX_STANDARD-from-14-to-17 branch from 099c723 to 0ec405c Compare March 18, 2023 15:40

N-Dekker mentioned this pull request Mar 18, 2023

COMP: Place #define _SILENCE_..._DEPRECATION_WARNING before #include #3970

Merged

N-Dekker force-pushed the CXX_STANDARD-from-14-to-17 branch from 0ec405c to 561d522 Compare March 18, 2023 21:51

dzenanz approved these changes Mar 20, 2023

View reviewed changes

N-Dekker mentioned this pull request Mar 21, 2023

STYLE: Remove std::allocator from TestImportImageContainer (Common/test) #3971

Merged

N-Dekker added 3 commits March 22, 2023 18:44

ENH: Upgrade ITK from C++14 to C++17

6b6ddf5

Follow-up to pull request InsightSoftwareConsortium#2563 commit daec0fd "ENH: Upgrade ITK from C++11 to C++14"

N-Dekker force-pushed the CXX_STANDARD-from-14-to-17 branch from 561d522 to 44eab79 Compare March 22, 2023 17:45

github-actions bot added the area:Documentation Issues affecting the Documentation module label Mar 22, 2023

N-Dekker force-pushed the CXX_STANDARD-from-14-to-17 branch from 44eab79 to 57fd767 Compare March 22, 2023 18:11

dzenanz reviewed Mar 22, 2023

View reviewed changes

Comment thread Documentation/SupportedCompilers.md

Comment thread Modules/Core/Common/include/itkMacro.h Outdated

N-Dekker force-pushed the CXX_STANDARD-from-14-to-17 branch from 57fd767 to de7124e Compare March 23, 2023 13:11

N-Dekker changed the title ~~WIP (still to be discussed): Upgrade ITK from C++14 to C++17~~ ENH: Upgrade ITK from C++14 to C++17 Mar 23, 2023

N-Dekker marked this pull request as ready for review March 23, 2023 16:50

N-Dekker mentioned this pull request Mar 29, 2023

COMP: Applied gtest PR 4025 to fix build with old clang #3978

Closed

dzenanz requested review from blowekamp, hjmjohnson, jamesobutler and thewtex March 29, 2023 16:42

dzenanz requested a review from jhlegarreta March 29, 2023 16:42

InsightSoftwareConsortium deleted a comment from kwrobot-v1 bot Mar 29, 2023

thewtex approved these changes Mar 30, 2023

View reviewed changes

dzenanz merged commit 45407f3 into InsightSoftwareConsortium:master Mar 30, 2023

N-Dekker mentioned this pull request Apr 2, 2023

ENH: Upgrade elastix from C++14 to C++17 SuperElastix/elastix#864

Merged

N-Dekker mentioned this pull request Apr 20, 2023

STYLE: Change some declarations to use trailing return type #4020

Merged

2 tasks

Uh oh!

Conversation

N-Dekker commented Mar 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

N-Dekker commented Mar 18, 2023

Uh oh!

issakomi commented Mar 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

N-Dekker commented Mar 18, 2023

Uh oh!

dzenanz commented Mar 20, 2023

Uh oh!

dzenanz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kwrobot-v1 bot commented Mar 29, 2023

Uh oh!

dzenanz commented Mar 29, 2023

Uh oh!

kwrobot-v1 bot commented Mar 29, 2023

Uh oh!

dzenanz commented Mar 29, 2023

Uh oh!

bradking commented Mar 29, 2023

Uh oh!

seanm commented Mar 31, 2023

Uh oh!

seanm commented Apr 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

N-Dekker commented Mar 17, 2023 •

edited

Loading

issakomi commented Mar 18, 2023 •

edited

Loading