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

Comparisons between large unsigned and negative signed integers #1295

Closed
julian-becker opened this issue Oct 12, 2018 · 1 comment
Closed
Labels
state: please discuss please discuss the issue or vote for your favorite option state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated

Comments

@julian-becker
Copy link
Contributor

When investigating more thorough tests for #1254, I stumbled across this unintuitive behavior:

TEST_CASE("UINT64_MAX shall not equal -1")
{
    json uint64max(static_cast<std::uint64_t>(UINT64_MAX));
    CHECK(uint64max.is_number_unsigned());

    json min1( static_cast<std::int64_t>(-1) );
    CHECK(min1.is_number_integer());

    CHECK(uint64max != min1); // <<--- THIS CHECK FAILS
}

My expectation would have been that uint64max != min1, but they compare equal on my machine, which is a MacBook Pro (Retina, 13", Late 2012) , using Apple LLVM version 9.1.0 (clang-902.0.39.2) for the target x86_64-apple-darwin17.6.0.

I have observed this behavior in the current develop branch and I have not checked the release branches.

The behavior - unintuitive as it may be - seems to stem from the fact that numbers INT64_MAX .. UINT64_MAX don't have a negative equivalent that fits into 64 bits, and UINT64_MAX and -1 both have the same binary representation (in 64 bits).

The failing test above seems to be in contradiction with the documentation on the comparison operator:

@brief comparison: equal

Compares two JSON values for equality according to the following rules:
- Two JSON values are equal if (1) they are from the same type and (2)
  their stored values are the same according to their respective
  `operator==`.

The values being compared here have both different type (is_number_unsigned() vs is_number_integer()) and different values (UINT64_MAX vs -1).

If a fix is desired, the following adjustment could be made to the implementation of basic_json::operator==():

else if (lhs_type == value_t::number_unsigned and rhs_type == value_t::number_integer)
{
    if (lhs.m_value.number_unsigned > static_cast<NumberUnsignedType>((std::numeric_limits<NumberIntegerType>::max)()))
    {
        return false;
    }
    return (static_cast<number_integer_t>(lhs.m_value.number_unsigned) == rhs.m_value.number_integer);
}
else if (lhs_type == value_t::number_integer and rhs_type == value_t::number_unsigned)
{
    if (rhs.m_value.number_unsigned > static_cast<NumberUnsignedType>((std::numeric_limits<NumberIntegerType>::max)()))
    {
        return false;
    }
    return (lhs.m_value.number_integer == static_cast<number_integer_t>(rhs.m_value.number_unsigned));
}

If the present behavior is actually intended, maybe consider adjusting the documentation to more clearly point out this potential pitfall.

@nlohmann nlohmann added the state: please discuss please discuss the issue or vote for your favorite option label Oct 16, 2018
@stale
Copy link

stale bot commented Nov 15, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label Nov 15, 2018
@stale stale bot closed this as completed Nov 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state: please discuss please discuss the issue or vote for your favorite option state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated
Projects
None yet
Development

No branches or pull requests

2 participants