-
Notifications
You must be signed in to change notification settings - Fork 54
Updates for C++17 compilation required on some systems linking against kwiver master #781
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
base: master
Are you sure you want to change the base?
Conversation
- Change fletch_BUILD_CXX11 option to fletch_BUILD_CXX17 with standard "17" - Add --extra-cxxflags="-std=c++17" to FFmpeg configure when enabled - Update OpenCV, VXL, libkml, and pybind11 to use fletch_BUILD_CXX17 - Add VXL patch file for vil_gauss_filter.cxx (original version) - Add libkml patch file for feature_list.cc (original version) - Update Patch.cmake files to conditionally apply C++17 patches - Update README documentation Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- VXL vil_gauss_filter.cxx: Replace deprecated std::bind2nd with lambda - libkml feature_list.cc: Remove deprecated std::binary_function inheritance These changes fix compilation errors when building with C++17, where std::bind2nd and std::binary_function were removed from the standard. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
daniel-riehm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense to have C++11 be the standard now that C++17 has superseded it, instead of the default still being C++98. Unless this causes a problem?
| endif() | ||
|
|
||
| if(fletch_BUILD_CXX17) | ||
| list(APPEND FFMPEG_CONFIGURE_COMMAND --extra-cxxflags="-std=c++17") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am confused here - isn't FFmpeg written in C, not C++?
| # OpenCV 3.3.0 has an option to enable C++ 11 | ||
| if (fletch_BUILD_CXX11) | ||
| if (fletch_BUILD_CXX17) | ||
| list(APPEND OpenCV_EXTRA_BUILD_FLAGS -DENABLE_CXX11:BOOL=ON) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is -DENABLE_CXX11 still correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On older versions, yes, there is no enable 17 so enable 11 is the closest thing and we still want to enable it when using 17
| add_custom_target(fletch-build-install) | ||
|
|
||
| # Include CXX11 support | ||
| set(fletch_CXX_STANDARD_VERSION "98") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| set(fletch_CXX_STANDARD_VERSION "98") | |
| set(fletch_CXX_STANDARD_VERSION "11") |
Does this seem reasonable?
| if (NOT fletch_BUILD_CXX11) | ||
| message(FATAL_ERROR "CXX11 must be enabled to use pybind11") | ||
| if (NOT fletch_BUILD_CXX17) | ||
| message(FATAL_ERROR "CXX17 must be enabled to use pybind11") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe remove this if the default standard is changed from 98 -> 11?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no objections to changing the default to 11 if that doesn't mess up any other projects. I will always use the 17 option so it doesn't matter
No description provided.