Issue146 misc fixes - #148
Open
almalkim wants to merge 13 commits into
Open
Conversation
set(int i) and clear(int i) modified digit[] directly but did not normalize the top digit afterwards. Without adjust_hod(), spare bits in the high-order digit retain stale sign-extension data, causing to_int()/to_uint() and other accessors to return wrong results. Fixes ticket_bitref_set_clear.
Author
|
Hi @AndrewGoodrich, |
Author
|
Hi @AndrewGoodrich, |
Contributor
|
These are good changes, thank you. A couple of suggested adjustments:
|
The previous implementation used remainder() which can return negative values, and omitted adjust_hod() leaving stale sign-extension bits in the high-order digit. New implementation: - truncates toward zero with std::trunc() - extracts digits using std::fmod/std::floor (always non-negative) - negates via two's complement for negative inputs - calls adjust_hod() to normalise the top digit Fixes ticket_op_assign_double.
Mirrors the same fix applied to sc_signed. The previous implementation used remainder() (which can return negative values) and omitted adjust_hod(), leaving stale bits in the high-order digit. New implementation: - truncates toward zero with std::trunc() - takes absolute value before digit extraction - extracts digits using std::fmod/std::floor - calls adjust_hod() to normalise the top digit Fixes ticket_op_assign_double.
…git[] get_packed_rep() writes raw sc_digit values from the caller's buffer into digit[] (the name is misleading — it actually sets, not gets). Without adjust_hod() the spare bits in the high-order digit are left un-normalized, so subsequent to_int()/to_uint() calls return stale values. Fixes ticket_packed_rep.
…(double) Two fixes in sc_unsigned.cpp: 1. set_packed_rep(): after writing bits from the caller's buffer into digit[], the high-order digit was not normalized. Added adjust_hod() so that spare bits above position (W-1) are always zeroed. Fixes ticket_packed_rep. 2. sc_unsigned_subref::operator=(double): rewrote digit extraction to use std::trunc/std::fmod/std::floor (same pattern as the signed fix) and added m_obj_p->adjust_hod() after vector_insert_bits() so the parent object's top digit is normalized. Fixes ticket_subref_assign_double.
almalkim
force-pushed
the
issue146_misc_fixes
branch
from
April 4, 2026 20:18
df61437 to
b645d9a
Compare
to_int64(): for 33 <= W < 64 the two-digit assembly produced a positive value even for negative inputs because the upper word only fills bits 32..(W-1), leaving the sign bit at position (W-1) rather than bit 63. Added arithmetic sign extension via shift-left/shift-right after assembly. Fixes ticket_to_int64.
When nbits is between 33 and 63, the two-digit assembly leaves the result sign bit at position (nbits-1) instead of bit 63. Added arithmetic sign extension via shift-left/shift-right, mirroring the same fix applied to sc_bigint<W>::to_int64(). Fixes ticket_to_int64.
almalkim
force-pushed
the
issue146_misc_fixes
branch
from
April 4, 2026 20:51
b645d9a to
5f6b99f
Compare
Author
|
All fixed |
Contributor
|
The shifts for signs propagation in to_int64() are not necessary because the internal values always contain sign-propagation in the unused upper bits. So an sc_bigint<40> will have its bits 40 through 63 as ones if it is a negative number. |
The adjust_hod() invariant guarantees that all bits above the value width are already sign-extended in the internal two's-complement representation, so the explicit shift-left/shift-right sign propagation in sc_bigint::to_int64() and sc_signed::to_int64() is redundant. Addresses reviewer feedback.
Author
|
Hi @AndrewGoodrich, |
Audit against clean upstream confirms only sc_signed bugs: - sc_signed set()/clear() missing adjust_hod() - sc_signed operator=(double) floor vs trunc and missing adjust_hod() - sc_signed get_packed_rep() missing adjust_hod() Reverts all sc_unsigned.cpp changes (no demonstrated bugs). Removes to_int64 and subref_assign_double tests (no bugs). Updates op_assign_double and packed_rep tests to only cover confirmed sc_signed bugs.
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
2 participants
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.
Fixes adjust_hod() missing after set/clear/packed_rep/subref, rewrites operator=(double), and corrects to_int64() sign extension.