Fix integer comparison bug by ljcjclljc · Pull Request #5211 · nlohmann/json

ljcjclljc · 2026-06-17T17:41:35Z

This PR provides a minimal implementation for #5210.
Fiex #5210

Background

When comparing number_unsigned and number_integer, the current implementation converts the unsigned value to number_integer_t before applying the operator.

This is problematic for values above INT64_MAX, because converting such uint64_t values to number_integer_t changes them into the negative signed range. As a result, valid mixed signed/unsigned comparisons can return mathematically incorrect results.

Problem

The affected logic is in the mixed comparison branches of JSON_IMPLEMENT_OPERATOR in include/nlohmann/json.hpp.

Before this change, comparisons such as these could fail:

json(-1) < json(18446744073709551615ULL) returned false
json(18446744073709551615ULL) == json(-1) returned true

This is not limited to one construction path. The same incorrect behavior can be observed after direct construction, object assignment, array insertion, initializer-list construction, nested access, and json::parse().

Solution

This PR keeps the existing macro shape and applies the smallest possible fix in the mixed number_unsigned / number_integer comparison branches:

If the signed operand is negative, return the mathematically correct ordering result directly.
If the signed operand is non-negative, cast it to number_unsigned_t and compare in the unsigned domain.

This avoids the unsafe uint64_t -> int64_t conversion while preserving the existing comparison structure.

Scope

This PR is intentionally minimal:

no refactor of the comparison macro
no change to the public API
no unrelated behavior changes

It only fixes the signed/unsigned mixed comparison bug described in #5210.

The changes are described in detail, both the what and why.
If applicable, an https://github.com/nlohmann/json/issues is referenced.
The https://coveralls.io/github/nlohmann/json remained at 100%. A test case for every new line of code.
If applicable, the https://json.nlohmann.me is updated.
The source code is amalgamated by running make amalgamate.

github-actions · 2026-06-17T18:01:54Z

🔴 Amalgamation check failed! 🔴

The source code has not been amalgamated. @ljcjclljc
Please read and follow the Contribution Guidelines.

gregmarr

This seems reasonable at a low level. There may be some cases that should be documented where a < b and json(a) < json(b) will provide different results. Those are the same cases where the compiler should have been warning you not to do that a < b, so that's probably okay, but good to call it out explicitly.

gregmarr · 2026-06-17T18:40:34Z

        return lhs.m_data.m_value.number_float op static_cast<number_float_t>(rhs.m_data.m_value.number_unsigned);     \
    }                                                                                                    \
-    else if (lhs_type == value_t::number_unsigned && rhs_type == value_t::number_integer)                \
+   else if (lhs_type == value_t::number_unsigned && rhs_type == value_t::number_integer)                \


You lost a space here.

gregmarr · 2026-06-17T18:41:11Z

+        {                                                                                                \
+            return number_integer_t(1) op number_integer_t(0);                                           \
+        }                                                                                                \
+        return lhs.m_data.m_value.number_unsigned op static_cast<number_unsigned_t>(rhs.m_data.m_value.number_integer); \


This is a good change, aligns more with the C++ promotion rules.

Signed-off-by: ljccjlljc <939159710@qq.com>

…r std_fs::path (nlohmann#5209) * added explicit instantiations of 'has_from_json' and 'has_to_json' for std_fs::path; Signed-off-by: drcosmin <cosmin.dr@pm.me> * Fixed amalgamation Signed-off-by: drcosmin <cosmin.dr@pm.me> --------- Signed-off-by: drcosmin <cosmin.dr@pm.me> Signed-off-by: ljccjlljc <939159710@qq.com>

Signed-off-by: ljccjlljc <939159710@qq.com>

github-actions · 2026-06-18T06:03:21Z

🔴 Amalgamation check failed! 🔴

The source code has not been amalgamated. @ljcjclljc
Please read and follow the Contribution Guidelines.

Signed-off-by: ljccjlljc <939159710@qq.com>

gregmarr

IMO, this needs a lot more documentation, both in the main documentation section, and somewhere that will show up in the changes section of the readme as a potentially breaking change, with the appropriate rationale, as described in the linked issue.

gregmarr · 2026-07-01T14:45:11Z

Should this be part of this PR, or separate? I am leaning towards separate.

Yes, please separate this from the original PR scope.

This change is already on develop branch. Please rebase.

gregmarr · 2026-07-01T14:45:25Z

Should this be part of this PR, or separate? I am leaning towards separate.

Yes, please separate this from the original PR scope.

This change is already on develop branch. Please rebase.

gregmarr · 2026-07-01T14:46:46Z

+        return lhs.m_data.m_value.number_unsigned op static_cast<number_unsigned_t>(rhs.m_data.m_value.number_integer); \
    }                                                                                                    \
-    else if (lhs_type == value_t::number_integer && rhs_type == value_t::number_unsigned)                \
+    else if (lhs_type == value_t::number_integer && rhs_type == value_t::number_unsigned) /* NOLINT(readability/braces) */ \


Rather than disabling the lint warning, why not just add the parens and make it clear what the precedence is?

Yes, please fix this.

gregmarr · 2026-07-01T14:55:02Z

+    // Mixed signed/unsigned integer comparisons check for negative signed
+    // values before casting. This avoids wraparound when converting a negative
+    // integer to an unsigned type and preserves the rule that any negative
+    // integer is smaller than any unsigned integer.


Since we probably don't want to put a comment inside the macro explaining the actual op, should there be one here?

// This is done by checking for the signed value being less than zero. // If it is, the comparison is performed using `0` for the signed value // and `1` for the unsigned value. Since the negative signed value will // always be less than the unsigned value, any two fixed values that // have the same relationship will produce the right result. // If the signed value is not less than zero, it is cast to unsigned // before doing the comparison.

I'm wondering if -1 and 1 would be clearer than 0 and 1. That way you can explicitly see the negative sign.

gregmarr · 2026-07-01T14:55:16Z

        return lhs.m_data.m_value.number_float op static_cast<number_float_t>(rhs.m_data.m_value.number_unsigned);     \
    }                                                                                                    \
-    else if (lhs_type == value_t::number_unsigned && rhs_type == value_t::number_integer)                \
+    else if (lhs_type == value_t::number_unsigned && rhs_type == value_t::number_integer) /* NOLINT(readability/braces) */ \


Rather than disabling the lint warning, why not just add the parens and make it clear what the precedence is?

Yes, please fix this.

nlohmann

Some comments. Will review later.

nlohmann · 2026-07-04T14:03:46Z

Yes, please separate this from the original PR scope.

nlohmann · 2026-07-04T14:03:50Z

Yes, please separate this from the original PR scope.

nlohmann · 2026-07-04T14:04:39Z

        return lhs.m_data.m_value.number_float op static_cast<number_float_t>(rhs.m_data.m_value.number_unsigned);     \
    }                                                                                                    \
-    else if (lhs_type == value_t::number_unsigned && rhs_type == value_t::number_integer)                \
+    else if (lhs_type == value_t::number_unsigned && rhs_type == value_t::number_integer) /* NOLINT(readability/braces) */ \


Yes, please fix this.

nlohmann · 2026-07-04T14:04:48Z

+        return lhs.m_data.m_value.number_unsigned op static_cast<number_unsigned_t>(rhs.m_data.m_value.number_integer); \
    }                                                                                                    \
-    else if (lhs_type == value_t::number_integer && rhs_type == value_t::number_unsigned)                \
+    else if (lhs_type == value_t::number_integer && rhs_type == value_t::number_unsigned) /* NOLINT(readability/braces) */ \


Yes, please fix this.

Signed-off-by: ljccjlljc <939159710@qq.com>

nlohmann · 2026-07-04T18:07:20Z

@@ -0,0 +1,19 @@
+#include <cstdint>


This file can now be removed given we have tests/src/unit-comparison.cpp.

nlohmann · 2026-07-04T18:08:33Z

This change is already on develop branch. Please rebase.

nlohmann · 2026-07-04T18:08:37Z

This change is already on develop branch. Please rebase.

nlohmann · 2026-07-04T18:09:49Z

+            const json max_uint64 = (std::numeric_limits<std::uint64_t>::max)();
+            const json negative_one = -1;
+
+            CHECK_FALSE(above_int64_max == negative_one);


The tests only check == and < but not <=, >, >=, !=, or operator<=>, and don't cover the non‑negative‑signed + large‑unsigned path. Please add these.

ljcjclljc requested a review from nlohmann as a code owner June 17, 2026 17:41

github-actions Bot added M tests labels Jun 17, 2026

gregmarr reviewed Jun 17, 2026

View reviewed changes

github-actions Bot added L M tests and removed M tests L labels Jun 17, 2026

ljcjclljc force-pushed the issue5210 branch from cae4d0e to 9956862 Compare June 18, 2026 02:45

ljcjclljc and others added 4 commits June 18, 2026 11:02

Fix integer comparison bug

81aa370

Signed-off-by: ljccjlljc <939159710@qq.com>

commit

2489cc8

Signed-off-by: ljccjlljc <939159710@qq.com>

Remove generated CI artifacts and update amalgamation

19b0af7

Signed-off-by: ljccjlljc <939159710@qq.com>

ljcjclljc force-pushed the issue5210 branch from 9956862 to 19b0af7 Compare June 18, 2026 03:03

Silence cpplint braces warning in comparison macro

80589f4

Signed-off-by: ljccjlljc <939159710@qq.com>

Update amalgamation after cpplint fix

08e7069

Signed-off-by: ljccjlljc <939159710@qq.com>

nlohmann requested changes Jun 28, 2026

View reviewed changes

Comment thread tests/src/uint64_compare_test.cpp

nlohmann linked an issue Jun 28, 2026 that may be closed by this pull request

Mixed comparison bug: number_unsigned (INT64_MAX+1~UINT64_MAX) vs number_integer #5210

Open

2 tasks

gregmarr reviewed Jul 1, 2026

View reviewed changes

ljcjclljc requested a review from nlohmann July 4, 2026 14:01

nlohmann reviewed Jul 4, 2026

View reviewed changes

Add mixed signed and unsigned comparison regression test

e556f9f

Signed-off-by: ljccjlljc <939159710@qq.com>

github-actions Bot removed the M label Jul 4, 2026

Clarify mixed signed and unsigned comparison handling

d3fe250

Signed-off-by: ljccjlljc <939159710@qq.com>

github-actions Bot added the M label Jul 4, 2026

nlohmann requested changes Jul 4, 2026

View reviewed changes

nlohmann added the please rebase Please rebase your branch to origin/develop label Jul 4, 2026

Merge branch 'nlohmann:develop' into issue5210

9678b3e

Uh oh!

Conversation

ljcjclljc commented Jun 17, 2026

Background

Problem

Solution

Scope

Uh oh!

github-actions Bot commented Jun 17, 2026

🔴 Amalgamation check failed! 🔴

Uh oh!

gregmarr left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jun 18, 2026

🔴 Amalgamation check failed! 🔴

Uh oh!

Uh oh!

gregmarr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nlohmann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gregmarr left a comment •

edited

Loading