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.

Sign up for GitHub

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

Jump to bottom

Check sign flag for i{8, 16, 32, ...} conversions #1867

Merged
merged 16 commits into from
Aug 22, 2023
Merged

Check sign flag for i{8, 16, 32, ...} conversions #1867

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
edcd6f3
Check sign flag for i{8, 16, 32, ...} conversions
jacogr Aug 21, 2023
4704647
Ensure negative path is tested on i32 perf
jacogr Aug 22, 2023
26dbfb4
CHANGELOG
jacogr Aug 22, 2023
ff9d02a
Comment check on top-most signed bit
jacogr Aug 22, 2023
54b65b0
toBn test array
jacogr Aug 22, 2023
26e33f0
toNumber uses exposed toBn tests
jacogr Aug 22, 2023
191029f
Re-use tests in toBn
jacogr Aug 22, 2023
31c1a9a
Re-use expectations inside bnToU8a
jacogr Aug 22, 2023
a847d66
Remove explicit BE (swap to non-optimal reverse)
jacogr Aug 22, 2023
a062e85
Adjust
jacogr Aug 22, 2023
0654290
Remove defunct "only BE" comment
jacogr Aug 22, 2023
270c10a
typo
jacogr Aug 22, 2023
5f523b3
Use bnToU8a tests in biToU8a
jacogr Aug 22, 2023
34e0ef7
Flatten tests
jacogr Aug 22, 2023
3abc4b5
Additional tests
jacogr Aug 22, 2023
2403866
Additional tests
jacogr Aug 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/util/src/u8a/toBigInt.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ export function u8aToBigInt (value: Uint8Array, { isLe = true, isNegative = fals
: value.slice().reverse();
const count = u8a.length;

if (isNegative && (!count || (u8a[count - 1] & 0x80))) {
if (isNegative && count && (u8a[count - 1] & 0x80)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So to understand this, isNegative will not indicate if we want to convert a signed value, correct?
If so maybe it would be good to refactor the name to isSigned and deprecate isNegative option?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The naming isNegative is indeed very unfortunate, it does mean isSigned and should have been named such 5 year ago. Got it right eventually in the api.

The timelines for deprecation (at least 6 months and then in the next major) and then the additional clutches it brings code & maintenance wise to support old/new has newer seemed to be worth the effort. So it is a wart.

switch (count) {
case 0:
return BigInt(0);
Expand Down
2 changes: 1 addition & 1 deletion packages/util/src/u8a/toBn.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ export function u8aToBn (value: Uint8Array, { isLe = true, isNegative = false }:

// shortcut for <= u48 values - in this case the manual conversion
// here seems to be more efficient than passing the full array
if (isNegative && (!count || (u8a[count - 1] & 0x80))) {
if (isNegative && count && (u8a[count - 1] & 0x80)) {
// Most common case i{8, 16, 32} default LE SCALE-encoded
// For <= 32, we also optimize the xor to a single op
switch (count) {
Expand Down
2 changes: 1 addition & 1 deletion packages/util/src/u8a/toNumber.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ export function u8aToNumber (value: Uint8Array, { isLe = true, isNegative = fals
// indicates a signed value, we use a two's complement conversion. If one of these
// flags are not set, we just do a normal unsigned conversion (the same shortcut
// applies in both the u8aTo{BigInt, Bn} conversions as well)
if (isNegative && (!count || (u8a[count - 1] & 0x80))) {
if (isNegative && count && (u8a[count - 1] & 0x80)) {
switch (count) {
case 0:
return 0;
Expand Down