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 always 64-bit arithemtics #245

Merged
merged 2 commits into from
Feb 17, 2025
Merged

Conversation

christoph-cullmann
Copy link
Contributor

long is only 32-bit on 32-bit systems and on 64-bit Windows ensure we have consistent behavior

long is only 32-bit on 32-bit systems and on 64-bit Windows
ensure we have consistent behavior
@CLAassistant
Copy link

CLAassistant commented Feb 13, 2025

CLA assistant check
All committers have signed the CLA.

@christoph-cullmann
Copy link
Contributor Author

See coin-or/Cbc#690

@jjhforrest

@jhmgoossens
Copy link
Contributor

The two Windows CI builds for "arch: x86_64" both fail with "error: 'int64_t' does not name a type;"
This seems to be caused by the stdint.h not getting automatically included via COINUTILS_HAS_STDINT_H.
Can this be fixed before merging?

@tkralphs tkralphs merged commit 328760e into coin-or:master Feb 17, 2025
13 checks passed
@christoph-cullmann
Copy link
Contributor Author

Thanks for fixing the missing header!

@jhmgoossens
Copy link
Contributor

@christoph-cullmann A little late, but can you resolve also the compiler Warnings about int64_t?

numerator_ = val;
CoinRational.cpp(32,18): warning C4244: '=': conversion from 'double' to 'int64_t', possible loss of data
denominator_ = 1.0;
CoinRational.cpp(33,20): warning C4244: '=': conversion from 'double' to 'int64_t', possible loss of data
numerator_ += std::abs(intpart) * denominator_;
CoinRational.cpp(86,35): warning C4244: '+=': conversion from 'double' to 'int64_t', possible loss of data

I guess an explicit cast to int64_t would be sufficient? or a round function?

These compiler warnings (and all others) are not shown in the Linux and Windows build logs using coinbrew, but show in the Windows MSVS build logs. At least instead of 254 such warnings we'd have a few less.

@christoph-cullmann
Copy link
Contributor Author

Mm, what is the right fix for that? Before there was the same data loss, even more, with the assignment to 32 bit or 64 bit ints. Does one just want a cast? Or does one want to die as it is erroneous if out of range?

@jhmgoossens
Copy link
Contributor

In all three cases, the actual values are integer anyway. For example, if (floor(val)==val) .... Therefore, a simple explicit cast is OK, like numerator_ = (int64_t) val;. I also added some tests in #246. The bigger remaining question for me is why the Cgl tests failed, but that's for Cgl.

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.

4 participants