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.

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

fix: fixed to be compatible with Flutter for Web (#8) #8

Merged
merged 4 commits into from
Jun 26, 2023
Merged

fix: fixed to be compatible with Flutter for Web (#8) #8

merged 4 commits into from
Jun 26, 2023

Conversation

myConsciousness
Copy link

Hi,

I implemented one solution mentioned in #7. However, I am not a very expert in this field, so perhaps the elimination of the mask operation may cause problem with the standard specifications. Please verify this point.

But at least my assumption was that if we guarantee the value of value variable as the maximum number within the allowable range of JavaScript, the mask operation would be unnecessary.

@myConsciousness myConsciousness changed the title fix: fixed to be compatible with Flutter for Web fix: fixed to be compatible with Flutter for Web (#8) Jun 26, 2023
@nelsonic nelsonic requested a review from LuchoTurtle June 26, 2023 01:57
@LuchoTurtle
Copy link
Member

LuchoTurtle commented Jun 26, 2023

Thanks for opening this issue!
I've looked at the changes you've made and they look fine to me! So they should be mergeable straight away. This code should correctly encode positive integers within the safe range supported by JavaScript as varints.

I was torn between doing something as you've done and using BigInt to represent this value, all the while also being compilable to 53-bit integers in JS. I implemented it with the masking because I wanted to have it more "generalized", but completely forgot about the JS bit constraint. If we wanted to encode integers that are larger than the safe integer range, we'd need to use BigInt, even with its performance cost. This is the "safer" option and will allow us to encode correctly any integer value, regardless of its size.

Since your code checks that the input value is within the safe integer range supported by JS and guaranteed to be non-negative, we know that the sign bit of the shifted value will always be 0, so no masking (&) is needed. This makes perfect sense.

I'll approve this PR and merge it.
Thank you! 😍

@codecov
Copy link

codecov bot commented Jun 26, 2023

Codecov Report

Merging #8 (5f9ab99) into main (21f74dd) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main        #8   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            4         4           
  Lines           54        57    +3     
=========================================
+ Hits            54        57    +3     
Impacted Files Coverage Δ
lib/src/multihash/varint_utils.dart 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@nelsonic nelsonic left a comment

Choose a reason for hiding this comment

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

Thanks. 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants