Skip to content

Conversation

@jonasschnelli
Copy link
Contributor

was introduced with #6121.

Fixes two warnings:

test/util_tests.cpp:353:64: warning: integer literal is too large to be represented in a signed integer type, interpreting as unsigned [-Wimplicitly-unsigned-literal]
    BOOST_CHECK(ParseInt64("-9223372036854775808", &n) && n == 9223372036854775808LL);

test/util_tests.cpp:353:61: warning: comparison of integers of different signs: 'int64_t' (aka 'long long') and 'unsigned long long' [-Wsign-compare]
    BOOST_CHECK(ParseInt64("-9223372036854775808", &n) && n == 9223372036854775808LL);

@laanwj
Copy link
Member

laanwj commented Jun 5, 2015

I don't get it. Why do you need unsigned integer here? If these are truly outside the 64 bit signed integer range, these tests should not be passing.
As I understand it, the range of int64_t should be -2^63 to 2^63-1, or .-9223372036854775808 to 9223372036854775807. These are the limits that I'm testing.
Am I mistaken?

Copy link
Member

Choose a reason for hiding this comment

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

OH wait! This should be n == -9223372036854775808LL (missed the -). There should be no need to make it unsigned, though.

@laanwj laanwj added the Tests label Jun 5, 2015
@jonasschnelli jonasschnelli force-pushed the 2015/06/util_test_clang branch from 0fd0194 to d0656d0 Compare June 5, 2015 18:36
@jonasschnelli
Copy link
Contributor Author

I really had my brain in "off mode" when i was writing this PR. I just added the same changes clang did warn about.
Now i have changed it from 9223372036854775807L to (int64_t)9223372036854775807 which should be the same IMO.
Fixed also the - sign in front of the min check (-9223372036854775808).

Still get a warning:
But it looks like that there is a bug in clang, because:
(int64_t)-9223372036854775808 -> shows warning (...is too large to be represented in a signed integer type...)
(int64_t)-9223372036854775807 -> OK
This is definitively wrong somehow.

@jonasschnelli jonasschnelli force-pushed the 2015/06/util_test_clang branch from d0656d0 to 40c7bd6 Compare June 5, 2015 19:27
@theuni
Copy link
Member

theuni commented Jun 6, 2015

I believe you need to use (int64_t)-9223372036854775807-1, otherwise it tries to negate 9223372036854775808, which itself doesn't fit to be negated.

*_MIN are usually defined that way, anyway:

#   define LLONG_MIN    (-LLONG_MAX - 1LL)

@laanwj
Copy link
Member

laanwj commented Jun 6, 2015

I believe you need to use (int64_t)-9223372036854775807-1, otherwise it tries to negate 9223372036854775808, which itself doesn't fit to be negated.

-9223372036854775807LL-1LL, then. I don't think you need the cast.

So I'm starting to understand. The C compiler itself uses 64 bit integers internally, and it parses the token '9223372036854775808' not -9223372036854775808. The value then overflows while parsing, in the end there's no difference but it likes to warn about it.

That means there is no way to represent the smallest integer without doing arithmetic, that's kind of stupid. But ok, fine with shutting up this warning, I just don't want another issue about this where it causes a warning in yet another compiler.

@jonasschnelli jonasschnelli force-pushed the 2015/06/util_test_clang branch from 40c7bd6 to c946ebe Compare June 6, 2015 08:12
@jonasschnelli
Copy link
Contributor Author

@theuni Okay. I see and understand this now better. Thanks.
Changed PR the follow the arithmetical way.

@laanwj
Copy link
Member

laanwj commented Jun 6, 2015

utACK

@laanwj
Copy link
Member

laanwj commented Jun 6, 2015

Ok, thanks, it passes travis. Going to merge this. If there is another compiler that starts to complain about that line we're just going to ignore it. We're writing software for users, not to make compilers happy.

@laanwj laanwj merged commit c946ebe into bitcoin:master Jun 6, 2015
laanwj added a commit that referenced this pull request Jun 6, 2015
c946ebe fix util_tests.cpp clang warnings (Jonas Schnelli)
zkbot added a commit to zcash/zcash that referenced this pull request Feb 5, 2017
…=<try>

Convert entire source tree from json_spirit to UniValue

This PR cherry-picks bitcoin/bitcoin#6121 and then migrates the Zcash-specific code to UniValue.

Also cherry-picks:
- bitcoin/bitcoin#6241
- bitcoin/bitcoin#6234

Closes #1985.
zkbot added a commit to zcash/zcash that referenced this pull request Feb 10, 2017
…=str4d

Convert entire source tree from json_spirit to UniValue

This PR cherry-picks bitcoin/bitcoin#6121 and then migrates the Zcash-specific code to UniValue.

Also cherry-picks:
- bitcoin/bitcoin#6241
- bitcoin/bitcoin#6234

Closes #1985.
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants