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

added CMake option USE_BOOST_INT128 to use a Boost.Multiprecision 128-bit integer for MathLib:big{u}int #7028

Merged
merged 1 commit into from
Dec 4, 2024

Conversation

firewave
Copy link
Collaborator

No description provided.

@firewave
Copy link
Collaborator Author

Utilizing this to have actual 128-bit support is a long way off.

As a first step this allows to iron out the inconsistent usage of the Mathlib types ({unsigned }long long instead of big{u}int) because the 128-bit types are actual types and not an alias so incompatible conversions will cause compilation failures. The first round of fixes without any impact will be in a follow-up PR.

Even if we cannot make it work in the end this would still provide a way for strong type safety we can leverage in the CI.

@danmar
Copy link
Owner

danmar commented Nov 21, 2024

As a complement.. if boost is not available and we compile cppcheck with gcc we could use __int128_t for bigints. However __int128_t does not support some math operations as far as I remember :-(

I am not sure if a 128-bit bigint is needed. At least not if our goal is only to handle all the 64-bit integers properly. Yes it is unfortunate to use "long long".
I believe that in clang/gcc they don't use a plain integer type to represent integer values like we do.

@danmar
Copy link
Owner

danmar commented Nov 21, 2024

The first round of fixes without any impact will be in a follow-up PR.
Even if we cannot make it work in the end this would still provide a way for strong type safety we can leverage in the CI.

ok that sounds good 👍

@firewave
Copy link
Collaborator Author

As a complement.. if boost is not available and we compile cppcheck with gcc we could use __int128_t for bigints. However __int128_t does not support some math operations as far as I remember :-(

And Visual Studio has not 128-bit support (yet).

I am not sure if a 128-bit bigint is needed. At least not if our goal is only to handle all the 64-bit integers properly. Yes it is unfortunate to use "long long".

It is - see https://trac.cppcheck.net/ticket/9994. And this was looked into a few years ago already: #3430.

@danmar
Copy link
Owner

danmar commented Nov 21, 2024

It is - see https://trac.cppcheck.net/ticket/9994.

I suggested here to use a class:
#3430 (comment)
that would be able to handle the value in 9994 properly because that is a 64-bit value.

The 128-bit integer has problems also. It won't automatically sign extend a signed 8-bit value properly or truncate 16-bit values properly. etc...

@firewave
Copy link
Collaborator Author

As mentioned above I am not yet looking into getting this working but start with the type safety as a baseline. If that is a given we essentially can replace it with anything - even dropping the whole boost build dependency. That should probably also help with identifying test cases (and possibly existing subtle issues).

…athLib:big{u}int` [skip ci]

Co-authored-by: chrchr-github <chrchr-github@users.noreply.github.com>
@@ -4,6 +4,8 @@ if(MSVC)
endif()
project(Cppcheck VERSION 2.16.99 LANGUAGES CXX)

include(cmake/options.cmake)
Copy link
Collaborator Author

@firewave firewave Nov 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to the top so the default values for the options will be applied here.

@@ -35,6 +39,14 @@ class CPPCHECKLIB MathLib {
friend class TestMathLib;

public:
#if defined(HAVE_BOOST) && defined(HAVE_BOOST_INT128)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved this because it will later be needed higher up in the class.

@firewave firewave changed the title added CMake option USE_BOOST_INT128 to use a 128-bit integer for MathLib:big{u}int added CMake option USE_BOOST_INT128 to use a Boost.Multiprecision 128-bit integer for MathLib:big{u}int Nov 21, 2024
@firewave firewave marked this pull request as ready for review November 23, 2024 15:33
@firewave firewave merged commit 74f9437 into danmar:main Dec 4, 2024
@firewave firewave deleted the bigint-2 branch December 4, 2024 22:40
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