improper use of MathLib::bigint

[Robert Reif]

We use a nonneg int for varid but use MathLib::bigint for exprid and pathid. MathLib::bigint should only be used for for handling parsed numeric values. I'm experimenting with using 128 bit integers for MathLib::bigint because we are getting daca crashes when trying to read 128 bit numbers using stoll which throws with 128 bit numbers. Is it really necessary to use 64 bit numbers for exprid and pathid? If so we should be using ssize_t because 64bit ids make no sense for 32 bit computers. Otherwise we should be using nonneg int like varid.

[Daniel Marjamäki]

I do not know if there is some good reason to use MathLib::bigint. But if not then using nonneg int sounds good to me.

I'm experimenting with using 128 bit integers for MathLib::bigint because we are getting daca crashes when trying to read 128 bit numbers using stoll which throws with 128 bit numbers.

I have missed those daca problems :-(

It would be very useful with 128-bit integers for our analysis and I have also thought about testing out 128-bit bigint..

[Robert Reif]

I looked at the code and expression ids start at the varid count. They don't occupy memory like varids but look like they are just used to keep track of the expressions. I guess that since varids used to be unsigned ints increasing the number required a 64 bit number. In reality since the token list has to fit in memory the maximum number of ids would be far smaller than MAX_INT. Since the token index is a int we don't expect the token count to ever exceed MAX_INT so it should be safe to use an int for the expression id. Otherwise we would be getting complaints of cppcheck crashing on 32 bit systems from running out of memory.

I have 128 bit bigint mostly working. The tests expect an InternalError or truncation with numbers greater than 64 bits but I'm not sure if we really want that. We could also add support for analyzing 128 bit integers if we wanted to.

[Daniel Marjamäki]

In reality since the token list has to fit in memory the maximum number of ids would be far smaller than MAX_INT.

yes I agree.

I have 128 bit bigint mostly working.

Interesting!

The tests expect an InternalError or truncation with numbers greater than 64 bits but I'm not sure if we really want that.

not sure which test you are talking about. But in theory in most cases it would be best with the same truncation that the running code has. however I do know that we do not match the running code 100%..

[Daniel Marjamäki]

however I do know that we do not match the running code 100%..

.. I do not expect that you fix this also in the same PR.

[Robert Reif]

Three failures are from numbers that overflow their limits. I haven't looked at the others yet.

Tests failed: 6

test/testcondition.cpp:3664(TestCondition::alwaysTrueInfer): Assertion failed. 
Expected: 
[test.cpp:2] -> [test.cpp:3]: (style) Condition '-128>x' is always false\n

Actual: 
[test.cpp:3]: (style) Redundant condition: If 'x < -128', the comparison 'x > 255' is always true.\n

_____
test/testcppcheck.cpp:92(TestCppcheck::getErrorMessages): Assertion failed. 
Expected: 

Actual: 
Duplicate ID: invalidScanfFormatWidth

_____
test/testmathlib.cpp:154(TestMathLib::calculate): Assertion failed. The expected exception was not thrown
_____
test/testtokenize.cpp:2872(TestTokenizer::simplifyKnownVariables57): Assertion failed. 
Expected: 
long long x ; x = -9223372036854775808L ;

Actual: 
long long x ; x = 9223372036854775808L ;

_____
test/testvalueflow.cpp:3050(TestValueFlow::valueFlowRightShift): Assertion failed. 
Expected: 
0

Actual: 
1

_____
test/testvalueflow.cpp:685(TestValueFlow::valueFlowCalculations): Assertion failed.

This is still WIP but I am submitting changes necessary to switch over to 128 bits when it's ready.

I'm considering putting bigint and biguint in a union in Value to get rid of confusing casts and possibly adding an overflow flag and an INT128 number type.