Repository navigation
Conversation
Signed-off-by: Juan Cruz Viotti <jv@jviotti.com>
|
This pull request is too large for Augment to review. The PR exceeds the maximum size limit of 75000 tokens (approximately 300000 characters) for automated code review. Please consider breaking this PR into smaller, more focused changes. |
Signed-off-by: Juan Cruz Viotti <jv@jviotti.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
7 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/runtime/encoder_number.cc">
<violation number="1" location="src/runtime/encoder_number.cc:56">
P2: `Decimal{"-9223372036854775808"}` has a coefficient equal to `INT64_MIN`, which satisfies the instruction's signed-64-bit condition, but this magnitude check rejects it; with assertions disabled, negating the converted value is undefined. Permit the extra magnitude for negative values and return `INT64_MIN` without negating it.</violation>
<violation number="2" location="src/runtime/encoder_number.cc:94">
P1: This rejects valid integer instances such as `JSON{5}`: `type: number` schemas compile to this instruction, and the fallback then calls `to_real()` on integers. Accept integer documents and encode their value as the coefficient with exponent zero.</violation>
<violation number="3" location="src/runtime/encoder_number.cc:103">
P1: `real_digits` casts the integral part directly to `int64_t`, so a finite real such as `1e20` triggers an out-of-range conversion instead of using a small coefficient and positive exponent. Normalize large real values into the tuple's coefficient and exponent before converting.</violation>
</file>
<file name="DEPENDENCIES">
<violation number="1" location="DEPENDENCIES:2">
P2: This Core pin makes Linux builds require OpenSSL 3.0 for Core crypto, although JSON BinPack does not link that library. Disable `SOURCEMETA_CORE_CRYPTO` in `FindCore.cmake` to keep builds working without OpenSSL.</violation>
</file>
<file name="src/runtime/encoder_any.cc">
<violation number="1" location="src/runtime/encoder_any.cc:120">
P1: This sends every arbitrary-precision decimal through a tuple encoder limited to 19 significant digits: larger valid values abort in assertion-enabled builds and can encode incorrectly otherwise. Extend the coefficient encoding to preserve wider decimals before routing all decimal values here.</violation>
</file>
<file name="src/runtime/include/sourcemeta/jsonbinpack/runtime_encoding.h">
<violation number="1" location="src/runtime/include/sourcemeta/jsonbinpack/runtime_encoding.h:293">
P3: `int64_t` does not hold every 19-digit coefficient; revise this to say that 19-digit values must also fall within the signed range.</violation>
</file>
<file name="src/runtime/decoder_number.cc">
<violation number="1" location="src/runtime/decoder_number.cc:55">
P2: This decodes valid finite values with negative exponents beyond 308 as zero: `(coefficient=1, exponent=-309)` overflows `scale`, so the result is `0` instead of `1e-309`. Scale the coefficient without overflowing the intermediate denominator.</violation>
</file>
Note: This PR contains a large number of files. cubic selects up to 200 of the highest-priority eligible files for this review, so some files may not have been reviewed.
View guided diff | Re-trigger cubic
| coefficient = decimal_coefficient(document.to_decimal(), exponent); | ||
| } else { | ||
| std::uint64_t point_position; | ||
| coefficient = sourcemeta::core::real_digits<std::int64_t>( |
There was a problem hiding this comment.
P1: real_digits casts the integral part directly to int64_t, so a finite real such as 1e20 triggers an out-of-range conversion instead of using a small coefficient and positive exponent. Normalize large real values into the tuple's coefficient and exponent before converting.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/runtime/encoder_number.cc, line 103:
<comment>`real_digits` casts the integral part directly to `int64_t`, so a finite real such as `1e20` triggers an out-of-range conversion instead of using a small coefficient and positive exponent. Normalize large real values into the tuple's coefficient and exponent before converting.</comment>
<file context>
@@ -19,4 +88,26 @@ auto Encoder::DOUBLE_VARINT_TUPLE(
+ coefficient = decimal_coefficient(document.to_decimal(), exponent);
+ } else {
+ std::uint64_t point_position;
+ coefficient = sourcemeta::core::real_digits<std::int64_t>(
+ document.to_real(), point_position);
+ exponent = -static_cast<std::int64_t>(point_position);
</file context>
| auto Encoder::NUMBER_VARINT_TUPLE( | ||
| const sourcemeta::core::JSON &document, | ||
| [[maybe_unused]] const struct NUMBER_VARINT_TUPLE &options) -> void { | ||
| assert(document.is_real() || document.is_decimal()); |
There was a problem hiding this comment.
P1: This rejects valid integer instances such as JSON{5}: type: number schemas compile to this instruction, and the fallback then calls to_real() on integers. Accept integer documents and encode their value as the coefficient with exponent zero.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/runtime/encoder_number.cc, line 94:
<comment>This rejects valid integer instances such as `JSON{5}`: `type: number` schemas compile to this instruction, and the fallback then calls `to_real()` on integers. Accept integer documents and encode their value as the coefficient with exponent zero.</comment>
<file context>
@@ -19,4 +88,26 @@ auto Encoder::DOUBLE_VARINT_TUPLE(
+auto Encoder::NUMBER_VARINT_TUPLE(
+ const sourcemeta::core::JSON &document,
+ [[maybe_unused]] const struct NUMBER_VARINT_TUPLE &options) -> void {
+ assert(document.is_real() || document.is_decimal());
+ const auto decimal{document.is_decimal()};
+ std::int64_t coefficient{0};
</file context>
| } else if (document.is_decimal()) { | ||
| this->put_byte(TYPE_OTHER | | ||
| static_cast<std::uint8_t>(SUBTYPE_DECIMAL << TYPE_SIZE)); | ||
| this->NUMBER_VARINT_TUPLE(document, {}); |
There was a problem hiding this comment.
P1: This sends every arbitrary-precision decimal through a tuple encoder limited to 19 significant digits: larger valid values abort in assertion-enabled builds and can encode incorrectly otherwise. Extend the coefficient encoding to preserve wider decimals before routing all decimal values here.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/runtime/encoder_any.cc, line 120:
<comment>This sends every arbitrary-precision decimal through a tuple encoder limited to 19 significant digits: larger valid values abort in assertion-enabled builds and can encode incorrectly otherwise. Extend the coefficient encoding to preserve wider decimals before routing all decimal values here.</comment>
<file context>
@@ -114,6 +114,10 @@ auto Encoder::ANY_PACKED_TYPE_TAG_BYTE_PREFIX(
+ } else if (document.is_decimal()) {
+ this->put_byte(TYPE_OTHER |
+ static_cast<std::uint8_t>(SUBTYPE_DECIMAL << TYPE_SIZE));
+ this->NUMBER_VARINT_TUPLE(document, {});
} else if (document.is_string()) {
const sourcemeta::core::JSON::String &value{document.to_string()};
</file context>
Comment on lines
+56
to
+72
| assert(magnitude <= | ||
| static_cast<std::uint64_t>(std::numeric_limits<std::int64_t>::max())); | ||
|
|
||
| auto adjusted_exponent{view.substr(marker + 1)}; | ||
| if (adjusted_exponent.front() == '+') { | ||
| adjusted_exponent.remove_prefix(1); | ||
| } | ||
|
|
||
| std::int64_t adjusted{0}; | ||
| [[maybe_unused]] const auto conversion{std::from_chars( | ||
| adjusted_exponent.data(), | ||
| adjusted_exponent.data() + adjusted_exponent.size(), adjusted)}; | ||
| assert(conversion.ec == std::errc{}); | ||
|
|
||
| exponent = adjusted - static_cast<std::int64_t>(digits - 1); | ||
| const auto coefficient{static_cast<std::int64_t>(magnitude)}; | ||
| return negative ? -coefficient : coefficient; |
There was a problem hiding this comment.
P2: Decimal{"-9223372036854775808"} has a coefficient equal to INT64_MIN, which satisfies the instruction's signed-64-bit condition, but this magnitude check rejects it; with assertions disabled, negating the converted value is undefined. Permit the extra magnitude for negative values and return INT64_MIN without negating it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/runtime/encoder_number.cc, line 56:
<comment>`Decimal{"-9223372036854775808"}` has a coefficient equal to `INT64_MIN`, which satisfies the instruction's signed-64-bit condition, but this magnitude check rejects it; with assertions disabled, negating the converted value is undefined. Permit the extra magnitude for negative values and return `INT64_MIN` without negating it.</comment>
<file context>
@@ -2,8 +2,77 @@
+ magnitude = (magnitude * 10) + static_cast<std::uint64_t>(digit - '0');
+ }
+
+ assert(magnitude <=
+ static_cast<std::uint64_t>(std::numeric_limits<std::int64_t>::max()));
+
</file context>
Suggested change
| assert(magnitude <= | |
| static_cast<std::uint64_t>(std::numeric_limits<std::int64_t>::max())); | |
| auto adjusted_exponent{view.substr(marker + 1)}; | |
| if (adjusted_exponent.front() == '+') { | |
| adjusted_exponent.remove_prefix(1); | |
| } | |
| std::int64_t adjusted{0}; | |
| [[maybe_unused]] const auto conversion{std::from_chars( | |
| adjusted_exponent.data(), | |
| adjusted_exponent.data() + adjusted_exponent.size(), adjusted)}; | |
| assert(conversion.ec == std::errc{}); | |
| exponent = adjusted - static_cast<std::int64_t>(digits - 1); | |
| const auto coefficient{static_cast<std::int64_t>(magnitude)}; | |
| return negative ? -coefficient : coefficient; | |
| assert(magnitude <= | |
| static_cast<std::uint64_t>(std::numeric_limits<std::int64_t>::max()) + | |
| static_cast<std::uint64_t>(negative)); | |
| auto adjusted_exponent{view.substr(marker + 1)}; | |
| if (adjusted_exponent.front() == '+') { | |
| adjusted_exponent.remove_prefix(1); | |
| } | |
| std::int64_t adjusted{0}; | |
| [[maybe_unused]] const auto conversion{std::from_chars( | |
| adjusted_exponent.data(), | |
| adjusted_exponent.data() + adjusted_exponent.size(), adjusted)}; | |
| assert(conversion.ec == std::errc{}); | |
| exponent = adjusted - static_cast<std::int64_t>(digits - 1); | |
| if (negative && | |
| magnitude == static_cast<std::uint64_t>( | |
| std::numeric_limits<std::int64_t>::max()) + | |
| std::uint64_t{1}) { | |
| return std::numeric_limits<std::int64_t>::min(); | |
| } | |
| const auto coefficient{static_cast<std::int64_t>(magnitude)}; | |
| return negative ? -coefficient : coefficient; |
| vendorpull https://github.com/sourcemeta/vendorpull 1dcbac42809cf87cb5b045106b863e17ad84ba02 | ||
| core https://github.com/sourcemeta/core 614dd74eb014f50b4b83d81db3a39de43ffe83d0 | ||
| blaze https://github.com/sourcemeta/blaze 87b62693c610f7a07efb63d1a8baf22e99571aa9 | ||
| core https://github.com/sourcemeta/core af2b932ef6284c880f47012749b78bb75526f7a4 |
There was a problem hiding this comment.
P2: This Core pin makes Linux builds require OpenSSL 3.0 for Core crypto, although JSON BinPack does not link that library. Disable SOURCEMETA_CORE_CRYPTO in FindCore.cmake to keep builds working without OpenSSL.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At DEPENDENCIES, line 2:
<comment>This Core pin makes Linux builds require OpenSSL 3.0 for Core crypto, although JSON BinPack does not link that library. Disable `SOURCEMETA_CORE_CRYPTO` in `FindCore.cmake` to keep builds working without OpenSSL.</comment>
<file context>
@@ -1,4 +1,4 @@
vendorpull https://github.com/sourcemeta/vendorpull 1dcbac42809cf87cb5b045106b863e17ad84ba02
-core https://github.com/sourcemeta/core 614dd74eb014f50b4b83d81db3a39de43ffe83d0
-blaze https://github.com/sourcemeta/blaze 87b62693c610f7a07efb63d1a8baf22e99571aa9
+core https://github.com/sourcemeta/core af2b932ef6284c880f47012749b78bb75526f7a4
+blaze https://github.com/sourcemeta/blaze a5a019aa49139e7cd21accd52ad2c65f66afff16
bootstrap https://github.com/twbs/bootstrap 1a6fdfae6be09b09eaced8f0e442ca6f7680a61e
</file context>
| double scale{1.0}; | ||
| for (std::uint64_t index = 0; index < shifts && std::isfinite(scale); | ||
| ++index) { | ||
| scale *= 10.0; |
There was a problem hiding this comment.
P2: This decodes valid finite values with negative exponents beyond 308 as zero: (coefficient=1, exponent=-309) overflows scale, so the result is 0 instead of 1e-309. Scale the coefficient without overflowing the intermediate denominator.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/runtime/decoder_number.cc, line 55:
<comment>This decodes valid finite values with negative exponents beyond 308 as zero: `(coefficient=1, exponent=-309)` overflows `scale`, so the result is `0` instead of `1e-309`. Scale the coefficient without overflowing the intermediate denominator.</comment>
<file context>
@@ -23,4 +26,38 @@ auto Decoder::DOUBLE_VARINT_TUPLE(
+ double scale{1.0};
+ for (std::uint64_t index = 0; index < shifts && std::isfinite(scale);
+ ++index) {
+ scale *= 10.0;
+ }
+
</file context>
Comment on lines
+293
to
+294
| /// number of up to 19 significant digits | ||
| /// |
There was a problem hiding this comment.
P3: int64_t does not hold every 19-digit coefficient; revise this to say that 19-digit values must also fall within the signed range.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/runtime/include/sourcemeta/jsonbinpack/runtime_encoding.h, line 293:
<comment>`int64_t` does not hold every 19-digit coefficient; revise this to say that 19-digit values must also fall within the signed range.</comment>
<file context>
@@ -265,6 +267,57 @@ struct ARBITRARY_MULTIPLE_ZIGZAG_VARINT {
+/// ### Conditions
+///
+/// The coefficient must fit in a signed 64-bit integer, which holds every
+/// number of up to 19 significant digits
+///
+/// ### Examples
</file context>
Suggested change
| /// number of up to 19 significant digits | |
| /// | |
| /// The coefficient must fit in a signed 64-bit integer, supporting all values | |
| /// with up to 18 significant digits and some values with 19. |
There was a problem hiding this comment.
5 existing issues remain and no new issues found across 338 files
Note: This PR contains a large number of files. cubic selects up to 200 of the highest-priority eligible files for this review, so some files may not have been reviewed.
Requires human review: Auto-approval blocked because this review re-detected 5 unresolved issues already reported by Cubic.
View guided diff | Re-trigger cubic
jviotti
changed the title
NUMBER_VARINT_TUPLE instructionNUMBER_VARINT_TUPLE encoding
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Juan Cruz Viotti jv@jviotti.com