search

Robbepop / apint

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

Crate reorganization and upgrade to edition 2018 #34

Closed AaronKutch closed 5 years ago

AaronKutch commented 5 years ago

Edit: I fixed some problems see my comment below

coveralls commented 5 years ago

Coverage Status

Coverage increased (+0.7%) to 75.714% when pulling ed241971daef36c8a5f276f971932dd9b3e581fa on AaronKutch:master into 418d8bfda90c019ca2bffc93ebc52718b5cc6068 on Robbepop:master.

coveralls commented 5 years ago

Coverage Status

Coverage increased (+0.7%) to 75.714% when pulling ed241971daef36c8a5f276f971932dd9b3e581fa on AaronKutch:master into 418d8bfda90c019ca2bffc93ebc52718b5cc6068 on Robbepop:master.

coveralls commented 5 years ago

Coverage Status

Coverage increased (+0.7%) to 75.714% when pulling 32adbbd39a03b6d76689c8ead2de0bbfa1173d40 on AaronKutch:master into 418d8bfda90c019ca2bffc93ebc52718b5cc6068 on Robbepop:master.

codecov-io commented 5 years ago

Codecov Report

Merging #34 into master will increase coverage by 0.74%. The diff coverage is 89.82%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #34      +/-   ##
==========================================
+ Coverage   74.97%   75.71%   +0.74%     
==========================================
  Files          22       30       +8     
  Lines        5062     5251     +189     
==========================================
+ Hits         3795     3976     +181     
- Misses       1267     1275       +8
Impacted Files Coverage Δ
src/data/digit_seq.rs 75% <ø> (ø)
src/info/radix.rs 88.88% <ø> (ø)
src/info/errors.rs 31.97% <ø> (ø)
src/construction/serde_impl.rs 79.38% <ø> (ø)
src/lib.rs 0% <ø> (ø) :arrow_up:
src/data/int.rs 0% <0%> (ø)
src/data/uint.rs 0% <0%> (ø)
src/construction/constructors.rs 99.47% <100%> (ø)
src/info/bitwidth.rs 98.27% <100%> (ø)
src/info/shiftamount.rs 100% <100%> (ø)
... and 35 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 418d8bf...32adbbd. Read the comment docs.

AaronKutch commented 5 years ago

On the "Completely Reorganize ..." commit, please only look at the diffs for the Cargo.toml and README.md files because the rest of the diffs are a complete mess. I suggest looking at not the commits but the resulting file structure and looking through all the files. You can also clone it and run cargo doc on it to look at how the documentation turned out.

AaronKutch commented 5 years ago

I experimented some and decided it wasn't worth it to add a feature that changes the Digit size to 32 bits. It would not be exceptionally hard however and I would like to communicate that as a note somewhere, along with removing the stuff in the readme that hinted at having a feature to change digit size.

AaronKutch commented 5 years ago

The next commit I want to make after you respond and I fix problems is to fix the constructors.

AaronKutch commented 5 years ago

I just went ahead and implemented the constructs needed to make the unchecked_internals flag work. The benchmarks are showing a ~40% difference in runtime (minus the allocation and cloning time). I am going to split this work into two commits, one for unary (only one ApInt involved) functions and a later one that affects binary function performance.

AaronKutch commented 5 years ago

With the help of the Borrow trait I have finally managed to get ApInt::from_iter_le(&[1u64,2,3,4,5]).unwrap() to work. It also works with Vecs so I get to remove that hacky from_vec_u64 function

Robbepop commented 5 years ago

I just went ahead and implemented the constructs needed to make the unchecked_internals flag work. The benchmarks are showing a ~40% difference in runtime (minus the allocation and cloning time). I am going to split this work into two commits, one for unary (only one ApInt involved) functions and a later one that affects binary function performance.

I know it might be a huge amount of work but could you please please (as I have already asked for before) split that thing into another PR? So this PR should be only concerned about the crate reorganization and 2018 upgrade and nothing else to keep it reviewable. The other PR can then maybe build on top of this PR and implement your proposed "unchecked_internals" of which I am still very suspicious to be honest. The problem with unchecked internals for me is that it goes against the nature of Rust of having unsafe for all things that avoid checking invariances and this proposal goes a completely different path by allowing this throughout the entire crate with a crate feature. There also are things like debug_assert that are only available in debug builds for testing and robustness purposes but they mostly do not check invariances and are just another safety guard for the implementors. So for me the "unchecked internals" sounds ultra dangerous and not suitable to the common Rust approach of doing things to be very honest. I deeply believe that we can achieve best possible performance with safe interfaces. And if a user really wants to drop all checks he or she can always choose to use unsafe interfaces.

Note that it is a feature of Rust to make potentially dangerous code really ugly (with unsafe).

Robbepop commented 5 years ago

So to summarize:

First of all, thanks a lot for all your hard work! I really appreciate it and also really want to merge all of it. :)

Please split this giantic PR into 2

  1. Upgrade to 2018 edition
  2. Your "unchecked internals" implementation

Then it will make it a lot easier to reason about (2.). In the current state of the PR I cannot really reason about the changes since it is really hard to go through nearly 5k lines of code just to find the critical parts of it which are obviously mainly the unchecked internals stuff. The crate upgrade shouldn't be too critical as far as I think.

AaronKutch commented 5 years ago

The code dealing with the unchecked_internals is in a separate commit already, so I will simply not push that commit to here. You can start reviewing what commits there are now, but remember I have not yet pushed commits like the new from_iter functions and the newest fuzz tester before this can be merged.

Robbepop commented 5 years ago

You can always split PRs and request a pull for them separately with a notification that they depend on some other PRs. If the unchecked_internals is already in a separate commit then it should be very easy to pull it out into its own PR. It would be very nice if you could do that. I really want to keep PRs self contained and minimal.

AaronKutch commented 5 years ago

The commit is stashed away on my local branch and I have not pushed it here yet, the commits you can currently see are ready for reviewing.

Robbepop commented 5 years ago

The commit is stashed away on my local branch and I have not pushed it here yet, the commits you can currently see are ready for reviewing.

ah okay sorry for the confusion!

AaronKutch commented 5 years ago

Is there anything you see problematic with the new module layout for you or newcomers who want to add commits?

Robbepop commented 5 years ago

As far as I remember it was fine in my opinion. However, I cannot tell if I recall every detail and if my view on the code base is actually comparable to that one of a newcomer.

AaronKutch commented 5 years ago

I have redone the work on a new branch to prevent confusion to myself, and have issued a new PR