Robbepop / apint

Arbitrary precision integers library.
Other
26 stars 4 forks source link

Issue 41 PR 2: Add `bw` function and clean the entire crate #61

Closed AaronKutch closed 3 years ago

AaronKutch commented 4 years ago

Don't be intimidated by this PR, because I made sure that it does not affect any logic, traits, or interfaces (except for adding the bw function and removing the BitWidth::wX() functions). There are no warnings now for any combination of features (except for fmt and clippy warnings that already existed). I reviewed almost every line of code in the crate and removed a few importing artifacts that remained from 2015 edition. I also fixed hard coding of Digit::BITS as a 64 in several places (there is still some hard coding surrounding parts of serialization and constructors, but this will require lots of cfgs no matter what if Digit::BITS is ever changed based on feature flags).

coveralls commented 4 years ago

Coverage Status

Coverage decreased (-0.2%) to 80.953% when pulling bc36124a0f89e1cffd6857ca16d6dd29e7f96c11 on AaronKutch:issue-41-bw into 343a3c7fbcacd8b8c0aa47fc52f58a2f3abc9639 on Robbepop:master.

codecov-commenter commented 4 years ago

Codecov Report

Merging #61 into master will decrease coverage by 0.17%. The diff coverage is 97.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #61      +/-   ##
==========================================
- Coverage   81.11%   80.93%   -0.18%     
==========================================
  Files          22       22              
  Lines        5131     5089      -42     
==========================================
- Hits         4162     4119      -43     
- Misses        969      970       +1     
Impacted Files Coverage Δ
src/apint/serde_impl.rs 76.29% <0.00%> (ø)
src/apint/utils.rs 79.42% <ø> (ø)
src/int.rs 18.47% <88.88%> (-0.52%) :arrow_down:
src/apint/casting.rs 72.72% <90.90%> (-0.12%) :arrow_down:
src/apint/serialization.rs 92.36% <94.44%> (-0.03%) :arrow_down:
src/bitwidth.rs 96.77% <95.00%> (-1.84%) :arrow_down:
src/apint/arithmetic.rs 93.43% <100.00%> (-0.02%) :arrow_down:
src/apint/bitwise.rs 88.94% <100.00%> (-0.30%) :arrow_down:
src/apint/constructors.rs 97.25% <100.00%> (-0.10%) :arrow_down:
src/apint/rand_impl.rs 92.20% <100.00%> (ø)
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 343a3c7...bc36124. Read the comment docs.

AaronKutch commented 4 years ago

Appveyor is failing because it cannot find rand for some reason

AaronKutch commented 4 years ago

I have a month and a half left in summer. We can't afford to do anything less than 1 PR per week.

AaronKutch commented 4 years ago

What do you think about the bw function? If it isn't something you want in the public API, I can put it behind a feature flag called "unstable".

Robbepop commented 4 years ago

What do you think about the bw function? If it isn't something you want in the public API, I can put it behind a feature flag called "unstable".

I am okay with it as const function but not otherwise.

AaronKutch commented 4 years ago

I can't make it const yet on stable. Is it OK if I put it behind the feature flag for now, or should I put off this PR?

Robbepop commented 4 years ago

I can't make it const yet on stable. Is it OK if I put it behind the feature flag for now, or should I put off this PR?

If it helps you put it behind a feature flag. However, in general please try to avoid those.

AaronKutch commented 4 years ago

I plan on using the same feature flag for any unstable SIMD stuff I want to do.

AaronKutch commented 4 years ago

And since it may be a while before the bw function is available to the public interface, I have added the constant width functions back in.

AaronKutch commented 4 years ago

Is there still something you do not like about this PR? Its been almost three weeks now. I think we have already missed the possibility of releasing 0.3.0 this summer.

AaronKutch commented 4 years ago

I just published specialized-div-rem 1.0.0. The compiler-builtins PR is practically done, but just needs some more review time and upstream fixes in LLVM.

AaronKutch commented 4 years ago

Thanks to more const stabilizations done in 1.46, I was able to make bw const. #[track_caller] was also stabilized which I added on. The only problem is that I had to work around the fact that regular panicking is not const stable yet, so I cause a panic by indexing outside of an array. This has a decent error message for const environments, but unfortunately the error message is very bad at runtime. Users should only be able to encounter problems, however, if they skipped all up front documentation that 0 bitwidths are not possible and the documentation on the function itself. The #[track_caller] isn't useful at the moment, but hopefully panicking in constants is allowed before 0.3.0 releases.

AaronKutch commented 4 years ago

I realized that the #[track_caller] is useless until I can panic normally with Location::caller(), so I have removed it. Also, it turns out that the hack compiles all the way back to 1.33.

codecov-io commented 3 years ago

Codecov Report

Merging #61 (343a3c7) into master (343a3c7) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #61   +/-   ##
=======================================
  Coverage   82.08%   82.08%           
=======================================
  Files          24       24           
  Lines        5146     5146           
=======================================
  Hits         4224     4224           
  Misses        922      922           

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 343a3c7...bc36124. Read the comment docs.

AaronKutch commented 3 years ago

The bot reminded me this PR was still up. I should say that I have moved on from apint and am trying to develop my own bitwidth-aware biginteger library. It has separated storage and reference capabilities that allow const big integer operation and mixed inline/external allocations. I should release it later this summer

Robbepop commented 3 years ago

The bot reminded me this PR was still up. I should say that I have moved on from apint and am trying to develop my own bitwidth-aware biginteger library. It has separated storage and reference capabilities that allow const big integer operation and mixed inline/external allocations. I should release it later this summer

Happy to hear that you are working on your own arbitrary precision library! Looking forward to see it emerge. :)

Robbepop commented 3 years ago

@AaronKutch I added a new GitHub Actions based CI to the repository. This PR no longer builds properly. If you are still interested in getting it merged please make sure to base it onto master. Otherwise please close this PR.