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

Switch to C++17? #2105

Closed
FlorianSchwendinger opened this issue Jan 7, 2025 · 4 comments
Closed

Switch to C++17? #2105

FlorianSchwendinger opened this issue Jan 7, 2025 · 4 comments
Assignees

Comments

@FlorianSchwendinger
Copy link

FlorianSchwendinger commented Jan 7, 2025

I would suggest to switch to C++17 since C++11 was released a long time ago and HiGHS already uses C++17 features here.

In HiGHS/app/cxxopts.hpp

#define CXXOPTS_FALLTHROUGH
#ifdef __has_cpp_attribute
  #if __has_cpp_attribute(fallthrough)
    #undef CXXOPTS_FALLTHROUGH
    #define CXXOPTS_FALLTHROUGH [[fallthrough]]
  #endif
#endif

fallthrough is used which seams to be available since C++17.

@jajhall
Copy link
Member

jajhall commented Jan 8, 2025

What's your view @galabovaa ?

@galabovaa
Copy link
Contributor

Not sure, it's only the runtime options which are not included in the library. Seems a bit silly to upgrade just for this.

Thank you @FlorianSchwendinger, I did not notice cxxopts have increased the C++ version when I updated our version of cxxopts with their latest one. I'm a bit worried we may break highs for users who are still using C++11 or 14.

Perhaps it is time I add the "library only" build option, so we can require c++11 if only the library is being built? But that doesn't sounds great either. We could just downgrade cxxopts back to when they used c++11 only, or find another command line parsing tool?

@jajhall
Copy link
Member

jajhall commented Jan 9, 2025

I agree with @galabovaa

@jajhall jajhall closed this as completed Jan 9, 2025
@FlorianSchwendinger
Copy link
Author

Build option

What I am doing now to aviod the

HiGHS/app/cxxopts.hpp:2303:5: warning: use of the 'fallthrough' attribute is a C++17 extension [-Wc++17-attribute-extensions]

message from CRAN is echo "" > HiGHS/app/CMakeLists.txt in my build script. So at least for me a option CMAKE file would be preferable. But what I am doing now should also work and is quite easy so there is no strong need for a option.

C++11 vs C++14 vs C++17

Some good news, the last version I submitted to CRAN I used already set(CMAKE_CXX_STANDARD 17) and it seamed to work (and is tested there at many platforms). But of course it could still break it for some users.

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

No branches or pull requests

3 participants