-
Notifications
You must be signed in to change notification settings - Fork 183
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
Extract Numerator and Denominator Constants to Byte Arrays #3922
Merged
Merged
Changes from all commits
Commits
Show all changes
110 commits
Select commit
Hold shift + click to select a range
1bda4d4
Add the needed helper and the structure
younies 8b97948
fix clippy
younies 1ac078a
change signature
younies ce27339
Add all the helpers functions
younies f785251
Use the helpers to generate the constant value
younies 2114087
fix
younies acedc32
first stage in data ( the data is not complete yet!)
younies b80be70
fix clippy
younies bf273b7
Merge branch 'main' of github.com:unicode-org/icu4x into units-conver…
younies 40a412c
fix fingerprint
younies e376ec6
Units conversion skeleton.
younies 8cd825b
finalize the skeleton
younies 0ab1091
update the data
younies 7fefed3
fix
younies 4c99aab
fix
younies ecd0df2
fix ule issue
younies 40f541e
Fix ULE issue and get the correct results
younies 51fb8a3
remove unnecessary print
younies 3330f09
fix assign
younies 10883bc
fix clippy
younies 5cbd0f2
improvement
younies b809232
improve
younies 27bbcc9
improve the code
younies 631b598
move to the correct place
younies 4a93af5
improve naming
younies 748d8e0
fix convert fractional
younies 0415d57
improve
younies 6c62c40
Fix comments and add tests
younies 4291b40
fix format
younies 3f8503e
improve and add test cases
younies a6248f3
fix clippy
younies eef1612
fix the comments
younies 50c03b0
rename
younies 42a7817
fix duplication
younies 346bd9a
fix the code
younies b5790d6
fix data bake
younies 8efed21
fix
younies c2ff685
fix
younies c7fb8da
fix clippy
younies 41beda1
Merge branch 'main' of github.com:unicode-org/icu4x into units-conver…
younies a52ca57
fix after merge
younies 754f33e
Update provider/datagen/src/transform/cldr/units/helpers.rs
younies 8cba11b
Add TODOs
younies c046110
fix the places of dependencies
younies f7467e6
fix uses
younies d098dfe
fix
younies 6c7cda6
fix tidy
younies edd6035
fix order
younies ea7e1bf
fix : cargo make ci-job-msrv-features-3
younies 408435d
Update experimental/unitsconversion/src/provider.rs
younies bcf11b1
make it more concise.
younies 9b2926a
rename a fn
younies eea4da0
make the removal more clear
younies 242fbf0
fix name
younies 9c27942
fix comments
younies a45cd8c
adjust function
younies e339d63
fix loop breaker
younies 20fac57
make the code concise
younies 1860cbd
make test concise
younies 4a10967
fix clippy
younies 940909e
fix naming
younies 8248b96
fix convert_constant_to_num_denom_strings
younies e965ce5
use BigRational
younies ac9daf5
fix fn name
younies 9af5b98
rename
younies 37bae97
fix
younies a107b75
fix the maximum depth
younies 9483c8f
fix
younies d1b022c
fix constant name
younies 8cc2a11
fix clibby
younies e781cf5
make the code more concise
younies 35ef822
fix fmt
younies 21ce45d
fix
younies e557ed2
remove the remove white space fn
younies 7e34f1f
improve
younies 9a60a86
fix
younies 364da65
fix clibby
younies bbed484
fix
younies 9553ef2
Update provider/datagen/src/transform/cldr/units/helpers.rs
younies c7365e4
fix
younies eea8a3e
make the function more concise
younies fe40b0e
add comments
younies 84fa812
fix
younies 010afc7
fix
younies 33fea6e
done fixing
younies 0fb142f
fix clibby
younies 63647c9
add issue
younies e734e14
remove dependency of "serde"
younies 45b3a1e
Merge branch 'main' of github.com:unicode-org/icu4x into units-conver…
younies e9c1a7c
fix after merge.
younies bcb706f
Merge branch 'main' of github.com:unicode-org/icu4x into units-conver…
younies 2398f8d
Use GenericFraction
younies 7324663
remove using vecs.
younies 78f03d5
fix clippy
younies e18ba12
detect the loop
younies ebfe084
detect the loop 2
younies a64515f
fix the loop
younies 1ca05fe
detect the loop 3
younies 36380c3
fix naming
younies 832aeeb
fix clippy
younies a5aa753
fix tidy
younies bb6b786
fix comment
younies d98c818
Merge branch 'main' of github.com:unicode-org/icu4x into units-conver…
younies afb8d8f
use &str instead
younies 1900832
c
younies ba1dde2
fix build after merge.
younies f5e2e79
rename count
younies 61796c6
Update provider/datagen/src/transform/cldr/units/mod.rs
younies 4a6e2bb
remove "num"
younies 70f48c7
add num
younies File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains 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
2 changes: 1 addition & 1 deletion
2
experimental/unitsconversion/data/data/macros/units_constants_v1.data.rs
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains 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
This file contains 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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: if you make one of the denominator/numerator signed, you only need a bit instead of an extra byte to store the sign.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not only the sign, it is still the
ConstantExactness
. Therefore, I prefer to add just a todoThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#4098
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exactness will be harder to optimise away, whereas the sign has an obvious solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having a signed numerator and an unsigned denominator is not a good idea. Consistency is beneficial for both maintenance and readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you know when deserializing whether the number is positive or negative?
Like, how do you know if 0xFF is +255 or -1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You deserialize with https://docs.rs/num-bigint/latest/num_bigint/struct.BigInt.html#method.from_signed_bytes_le?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation of that function seems nontrivial; it involves copying the bytes to a new vector and complementing them
https://docs.rs/num-bigint/latest/src/num_bigint/bigint/convert.rs.html#381
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think how the library serializes negatives is just by making the high bit in the highest nonzero byte be 1. But this means we lose capacity. For example, I imagine that you need to store 9 bytes for 255 (8 for the mantissa and 1 for the sign). You can't get out of storing the sign bit somewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I assumed
BigInt
would use 2s complement internally, but it uses a sign bit as well. I guess this aligns with what Younies is currently doing.Note that
from_bytes_le
also allocates, it's not possible to create a borrowedBigUint
.