Repository navigation
[BitCollections] sign extend BitArray conversions to integers wider than a word - #808
Conversation
…han a word converting a negative bit array to `Int128` lost the sign above the first 64 bits , so `Int128(truncatingIfNeeded: "1" as BitArray)` gave 18446744073709551615 , not -1 , `Int128(exactly:)` gave nil , and `Int128(_:)` trapped . a negative value now starts out as all ones , so the words shifted in keep the sign . the trap message also named `BitArray` as the type it could not convert to , it names the integer type now .
mangeshraut712
left a comment
mangeshraut712
left a comment
There was a problem hiding this comment.
I traced _foreachTwosComplementWordDownward (sign-extends the top word via _Word(upTo: end.bit).complement() for negative values) against the new ~.zero prefill: for negative values the prefill survives only in types wider than the array, which is exactly the case being fixed, and it is flushed out by the <<= UInt.bitWidth shifts for equal/narrower widths. I also re-verified _convertExactly's guard (isNegative == (value < 0)) and word-replay loop still agree with the new sign extension for cases like "1" + zeros(127) -> .min and ones(128) -> -1/.max. Nice fix — the \(Self.self) -> (T.self) correction in the fatalError message is a good catch too. One non-blocking suggestion inline.
| var value: T = .zero | ||
| // Negative values start out with all bits set, so that they remain sign | ||
| // extended in types that are wider than the words we shift in. | ||
| var value: T = T.isSigned && self.last == true ? ~.zero : .zero |
There was a problem hiding this comment.
Verified: this prefill only affects signed conversions where T is wider than the bit array, since value <<= UInt.bitWidth in the closure flushes all-ones out of any narrower type. Together with the sign-extended top word from _foreachTwosComplementWordDownward, Int128(truncatingIfNeeded: bits("1" + zeros(63))) correctly gives -2^63 rather than +2^63.
Non-blocking suggestion: the doc comments for init(truncatingIfNeeded:) / init?(exactly:) still only show same-width examples (e.g. Int8 cases). Since this PR introduces widening sign-extension semantics, one example pair like Int128(truncatingIfNeeded: "1" as BitArray) == -1 and Int128(exactly: "0" + ones(127)) == .max would lock in the new behavior for future readers.
There was a problem hiding this comment.
thanks for checking it . the examples already cover this , Int8(truncatingIfNeeded: "1" as BitArray) is -1 , a 1 bit array sign extended to 8 bits . the pull request does not change what the docs say , it only makes types wider than a word follow it , so i would leave the examples as they are . the new test pins the Int128 and UInt128 cases .
converting a negative
BitArrayto an integer type wider thanUIntlost the sign above the first word ._convertstarts from zero and shifts each word in , and a word is unsigned , so nothing ever sets the bits above it .on a 32 bit platform
UIntis 32 bits , soInt64would go down the same path . i did not run that one .the fix starts a negative value out as all ones , so the words shifted in keep the sign . narrower types are not affected , the shift drops those bits like before . the trap message also named
BitArrayas the type it could not convert to , it names the integer type now .the new test covers
Int128andUInt128around the 64 and 128 bit edges , and fails before the change . the existing conversion tests only useInt8andUInt8. i also ran a random check of the conversions toInt8,UInt8,Int,UInt,Int128andUInt128against a plain[Bool], and only the signed 128 bit ones were off . all 123BitCollectionsTestspass .Checklist