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

Use isfinite and isnan provided by <cmath> #231

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

a-andre
Copy link
Contributor

@a-andre a-andre commented Jul 19, 2024

The included provides std::isfinite and std::isnan. There is no need to fallback to C functions.

a-andre added 2 commits July 19, 2024 14:19
The included <cmath> provides std::isfinite and std::isnan. There is no
need to fallback to C functions.
@svigerske
Copy link
Member

The included provides std::isfinite and std::isnan.

With C++11 it became part of the standard, but before that?

There is no need to fallback to C functions.

But is there a harm in doing so?

@a-andre
Copy link
Contributor Author

a-andre commented Jul 20, 2024

It seems I misinterpreted the configure file and assumed C++11 or newer was already enforced.

Anyway, C++11 seems well supported nowadays. So why not required it for the current development version of CoinUtils and simplify code where possible?

@svigerske
Copy link
Member

I recall some discussions where a C++11 dependency was introduced the first time, and the decision back then was to not make it a requirement. In my recollection, the argument was that it would only be worth to go for C++11 if the whole code base would be cleaned up to use C++11 features. Increasing the C++ requirement just to save a few lines in CoinFinite.cpp wouldn't seem worth it.

@a-andre
Copy link
Contributor Author

a-andre commented Jul 21, 2024

Based on 1a83f9b I assume the discussion happened 5 years ago. Things might have changed since then.

If there is a probability of being accepted, I can looking into making more use of C++11 (or newer?) features.

@svigerske
Copy link
Member

Things might have changed since then.

Not in this corner of COIN-OR.

If there is a probability of being accepted, I can looking into making more use of C++11 (or newer?) features.

I don't think there is anything good at requiring higher C++ versions just because one could. If it were to provide some new interesting functionality or a (measurable) gain in performance, then that would be an argument. Also, if you wanted to take over the development of this project, then I would understand if you want a cleaner code base to work with.

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