Reorganize the entire crate's structure

[AaronKutch]

I have had a lot of thinking over the past month about apint, and I think I will put most of it here. These are things I had off the top of my head that I had to type down, maybe I will remember more.

Calling BitWidth::new(...).unwrap() just to handle the 0 width case outside the function might seem to have barely any performance increase when dynamically setting a variable with it and then calling several constructors with that one variable, but I think doing many small things like it will make a significant difference. In benchmarks I just did, we are outperforming ramp by about 0.4x on basic ops, and I think it is because of many small branching choices I am making in the new arithmetic.rs and because of all your small design choices working together to make a big difference. For brevity though, I think we should make a macro, perhaps bw!(), and make it a procedural macro so that it produces compilation errors on 0. We could also use the procedural macro crate for future compile time stuff. BitWidth::new() should be kept for dynamic bit size purposes, and I wonder if we should get rid of the other BitWidth constructors since they have the unwrap inside them, and I much prefer to be able to see unwraps and mess with Results. We obviously cannot do the same thing with ShiftAmount but we should keep that, BitPos, and Radix for purposes of easily distinguishing between widths and positions and shifts and them having special purpose methods. Thorough documentation could also be included for each of these types later.

I had some trouble getting around the crate for a long while, and I think making the following organizational changes would help alot. Make sure that all tests pass before doing this, change to edition = 2018, cargo fix it

• Keep the stuff in uint.rs and int.rs where they are. They are just complicated wrappers around ApInt and almost no impls for them should be anywhere else.
• mod.rs should be changed to apint.rs. The extremely unsafe and important stuff like the Drop impls in constructor.rs and the Clone impl in casting.rs should be moved here.
• The debug and hash impls in apint/utils.rs should be moved to serialization.rs
• The is_one, is_even, etc in apint/utils.rs should be put in relational.rs
• The outer utils.rs should be merged with apint/utils.rs
• I would take the impls for ShiftAmount and what is in checks.rs and put them in a new file shiftamount.rs.
• Then put shiftamount.rs, bitpos.rs, bitwidth.rs, radix.rs, traits.rs, and error.rs in their own folder, maybe called info.
• apint.rs,int.rs, uint.rs, digit.rs, digit_seq.rs are put into a folder called data
• arithmetic.rs, casting.rs, constructors.rs, relational.rs, shift.rs,bitwise.rs, and utils.rs are put into a folder called logic
• rand_impl.rs, serialization.rs, serde_impl.rs, to_primitive.rs, lib.rs, and the folders are what remain at the top level of src

A while ago I raised an issue about the Clone impl you wrote for apint but benchmarks show that it is performing as quickly as what ramp has, so unless we happened to end up with a same less than optimal cloning routine, it is good.

I just realized something. The impl for ApInt looks like:

/// An arbitrary precision integer with modulo arithmetics similar to machine integers.
pub struct ApInt {
    /// The width in bits of this `ApInt`.
    len : BitWidth,
    /// The actual data (bits) of this `ApInt`.
    data: ApIntData
}

union ApIntData {
    /// Inline storage (up to 64 bits) for small-space optimization.
    inl: Digit,
    /// Extern storage (>64 bits) for larger `ApInt`s.
    ext: NonNull<Digit>
}

How does this work? wouldn't the ext have to be NonNull<Storage>?

[AaronKutch]

I know from benchmarking now what I need to work on (currently everything below 512 bits is superior to ramp except for division). I think the next steps are for me to finish the initial implementing unimplemented! in division functions commit, then you and me can work in parallel (most of the next work I will be doing will involve messing with the insides of functions and not with library layout which you can work on and it will be easy to merge commits). I will get the commits straightened out and hopefully this weekend is when you can start working on apint again. I read a comment I made on May 1 about getting multiplication and division working (I probably thought the time frame was only 1 month or less then), wow I was so wrong. If I add simd and other things onto the already strong base we have, we are on track to outperform everything for medium sized integers, maybe even GMP itself.

[AaronKutch]

Do you have enough time to do this refactoring by the end of November? I really want to have this refactoring done with #![feature(rust_2018_preview)] before December 6 even if that means the division function still has bad performance. If you don't have enough time, then I will do the refactoring. Also, don't release any more releases until I have gone through my checklist. When you accepted my pull request for the arithmetic rewrite I actually didn't intend for you to immediately release but I decided later that it was good enough.

[Robbepop]

This is some awesome information btw!

I like your checklist and I am also strongly favoring the proposed restructuring of apint in general.
Having started a new job I cannot tell when I have enough time for the entire revamp of the crate but I am motivated to do the work that has to be done in apint.

In the next days I will create a proper 0.3 release tracking issue where we collect everything that has to be done to channel our efforts.

It is also awesome to hear that apint currently outperforms ramp in some scenarios. I have never designed apint for that and it is funny to hear that now. So let's make it perfect then! Maybe I can even find use cases for apint in my company's codebases. That would be really epic.

Thanks for all your work!
Are you okay if I mention you as co-author of the crate with my next commit on it?

[AaronKutch]

Its ok if you make me coauthor.
One key place where ramp outperforms apint is whenever the inputs are multiple Digits (or limbs in ramp's own terminology) but the output fits in just one single Digit. I panicked when I first saw my division benchmark results vs ramp, with ramp being slightly faster. If instead I made the division to have a quotient of more than one digit, then we outperform ramp up to about 192 bits.
In the future, I think I will implement a division function with a more generic signature similar to GMP's and other libraries that allows differently sized ApInt inputs. It will also include two ApInts to use as internal buffers to give users maximum performance. This should solve any sizing performance issues.

I just realized that ramp might not be using Knuth's algorithm, it just has functions named similar to the ones I see in the c++ GMP library and says it uses some kind of school book implementation. Either way, I am making an optimized function that uses Knuth's for my specialized_div_rem crate to make sure that my function still outperforms Knuth's for medium sized integers (EDIT: to be clear, Knuth's outperforms my algorithm pretty quickly after ~128-256 bits, but hey I made an improvement in that area). Then I will open an issue for Rust to use my algorithm on 128 bit integer division. Then I will use what I learn from that to fix the perf in my more general function used in apint. Later this Christmas break I will make a proper Knuth's algorithm for larger integers in apint.

[AaronKutch]

wait did you not see this issue until now?

[Robbepop]

About your question in the beginning:
The NonNull<Digit> is very similar to a *mut Digit that cannot be NULL. This isn't even required because currently Rust cannot use the non-null guarantee for further optimizations. However, it still is good for documenting the purpose of the thing.
So the entire union is either a Digit (64 bits) in the inl case or a *mut Digit (32 or 64 bits).

Btw.: I am also greatly in favor of moving to Rust 2018 as soon as possible - I think the next Rust release 1.31 will be Rust 2018 already. That's about in 6 weeks from now.

Reeeeeeeally looking forward to your div/rem implementations. Do we have a useful set of performance tests, yet? Would be great to have a benchmark comparing ramp, bigint, apint and maybe even GMP for some operations and bitwidths.

[AaronKutch]

Yeah I should make a file just for benchmarks and preventing perf regressions between releases. I have a partial one already that includes ramp and bigint. There is the gmp bindings crate, but that requires linking to c++, and I don't know how to go about that.

[Robbepop]

Comparisons to ramp and bigint would be awesome already if we had them. :)
And yeah, performance regressions are bad and we really should find ways (bench suite?) to avoid them.

[AaronKutch]

The beta for Rust 2018 is ready to go, I would start compiling with it.

[Robbepop]

Yeah you can do that. But keep in mind that we cannot release a new version of Apint as long as Rust 2018 ist not stable. It will be stabilized in approx. 6 weeks.

[AaronKutch]

EDIT: I figured out that the errors happen when compiling with default-features=false, we can fix it in the reorganization.
I am getting errors when trying to compile apint 2.0 that I'm pretty sure I didn't get before, only warnings I think. How close are you to finishing reorganizing?

[Robbepop]

EDIT: I figured out that the errors happen when compiling with default-features=false, we can fix it in the reorganization.

Ah that's a good hint!

I am getting errors when trying to compile apint 2.0 that I'm pretty sure I didn't get before, only warnings I think.

What do you mean by apint 2.0 ?

How close are you to finishing reorganizing?

I get the idea that there is some kind of misunderstanding. I thought I shall wait for the reorganization of modules until you are done with your current task. So I had lots of ideas for apint, like no_std support but didn't do anything, yet.

[AaronKutch]

I thought I shall wait for the reorganization of modules until you are done with your current task.

Have you not done anything in the last month? I said that you can work in parallel with me because I was fixing performance problems inside functions and you were reorganizing where the functions are. Then when you were done with your commits it would be easy for me to replace the insides of the functions in a later PR. That was one of the points of the last PR I made.

What do you mean by apint 2.0 ?

Whoops I meant 0.2.0, that's the current version of apint on crates.io.

[AaronKutch]

I almost forgot to say that along with the crate reorganization, you can maybe change some things about the error type in the crate. I think that message and annotation part of the Error struct should not be there taking up space between when it is generated and when it is displayed, and it should only be generated as part of displaying the Error.

[Robbepop]

I almost forgot to say that along with the crate reorganization, you can maybe change some things about the error type in the crate. I think that message and annotation part of the Error struct should not be there taking up space between when it is generated and when it is displayed, and it should only be generated as part of displaying the Error.

That's a good point and I should really do that!

Have you not done anything in the last month? I said that you can work in parallel with me because I was fixing performance problems inside functions and you were reorganizing where the functions are. Then when you were done with your commits it would be easy for me to replace the insides of the functions in a later PR. That was one of the points of the last PR I made.

Nope, as I have mentioned there was this slight misunderstanding. Even though I had ideas I postponed their implementation and did other stuff. Good to know!

[AaronKutch]

Rust 2018 is out! Make sure to run rustup self update first, then rustup update, finally make sure that you have a edition = "2018" key in Cargo.toml.

[AaronKutch]

You can run stuff like rustup component add rustfmt and add stuff like #[rustfmt::skip] (make sure to do that on functions with long ApInt::from([...])) and #[allow(clippy::bool_comparison)] to the code.

[Robbepop]

Yeah we really should use 2018 edition for all future updates.
It is awesome to work with it. :)

[AaronKutch]

My winter break is most of the way over already, how are you doing?

[Robbepop]

Nice that you ask!

My winter break is also almost over, have been some nice days.
I am doing fine but have lots of work to do for work until end of january or beginning of february.
So I cannot really find useful time for hobby projects at the moment.

[AaronKutch]

I just discovered I have two extra weeks to work on apint. If you have not done any reorganization I will do it, because I need it to be able to do more work.

[Robbepop]

Do whatever needs to be done. I trust you and your work and I will review it asap!
I will myself continue working actively on ApInt as soon as I find time for it.

[AaronKutch]

I prepared some benchmarking and discovered that in my past benchmarking I was causing ramp to reallocate most of the time. When I use the right inputs ramp actually outperforms apint. However, I have also been researching how GMP works and have put together a plan to fix more performance problems.

Some indexing bounds checks are eliminated in compilation but not all of them. In my digit wise rotate function I replaced all the indexing with .get_unchecked_mut and got about a 15% speedup which made it outperform the std slice rotate function up to 32 Digits rather than the 16 Digits previously. I think the conditional jump used in slice bounds checking interferes heavily with pipelining in other functions which could lead to massive performance increases elsewhere if that bounds checking was turned off. Of course, nothing in our crate is formally proven or anything but I want users who trust the fuzz testing and want speed to be able to turn internal checks off. Also, if the user is confident that their side is correct, the inputs to the crate functions could be set to unchecked. I propose adding two feature flags to the crate: "unchecked_internals" and "unchecked_input" which do these things.

ContiguousDigitSeq is barely used currently and could be renamed to DigitSeq and its internals changed so that it has two implementations of std::ops::Index, one of which uses the bounds checked indexing of the internal slice, and another which uses the get_unchecked, turned on by the "unchecked_internals" flag. DigitSeq will not be an iterator but rather a wrapper around &[Digit]. I have decided I have no use for iterators in this crate, even for basic shift functions because I will instead define common methods like add with carry for DigitSeq (to both reduce code repition throughout the crate and allow for the return of ll.rs and an assembly flag but that will come in a later refactor).

I will change arithmetic functions that use &[Digit] to rather use DigitSeq so my functions which heavily use indexing can be affected by the flag. Some functions like the basic shift functions will be broken by this but I planned on improving their performance anyways by rewriting them to index form.

So to summarize, I plan on doing the following, which I don't think is too much for one refactor:
*the reorganization
*the change to DigitSeq
*improving documentation
*maybe also do error improvements

[Robbepop]

I prepared some benchmarking and discovered that in my past benchmarking I was causing ramp to reallocate most of the time. When I use the right inputs ramp actually outperforms apint. However, I have also been researching how GMP works and have put together a plan to fix more performance problems.

Ah sorry to hear that! So one can say that depending on the use-case either apint or ramp is faster?

Some indexing bounds checks are eliminated in compilation but not all of them. In my digit wise rotate function I replaced all the indexing with .get_unchecked_mut and got about a 15% speedup which made it outperform the std slice rotate function up to 32 Digits rather than the 16 Digits previously. I think the conditional jump used in slice bounds checking interferes heavily with pipelining in other functions which could lead to massive performance increases elsewhere if that bounds checking was turned off. Of course, nothing in our crate is formally proven or anything but I want users who trust the fuzz testing and want speed to be able to turn internal checks off. Also, if the user is confident that their side is correct, the inputs to the crate functions could be set to unchecked. I propose adding two feature flags to the crate: "unchecked_internals" and "unchecked_input" which do these things.

As far as I understood you plan to introduce some abstractions to avoid safety checks in order to improve performance. I am okay with this as long as this doesn't allow users to accidentally or intentionally invalidate invariants of abstractions while using safe APIs. Functions or other abstractions that might accidentally or intentionally allow invalidating such invariances shall always be marked as unsafe. That's an inherent nature of Rust and also an inherent design goal of apint. I am very faithful that you can always achieve great performance and efficiency by always sticking to the rules of Rust of providing safe and unsafe abstractions - however, unsafe abstractions always need to be marked as such.

ContiguousDigitSeq is barely used currently and could be renamed to DigitSeq and its internals changed so that it has two implementations of std::ops::Index, one of which uses the bounds checked indexing of the internal slice, and another which uses the get_unchecked, turned on by the "unchecked_internals" flag. DigitSeq will not be an iterator but rather a wrapper around &[Digit]. I have decided I have no use for iterators in this crate, even for basic shift functions because I will instead define common methods like add with carry for DigitSeq (to both reduce code repition throughout the crate and allow for the return of ll.rs and an assembly flag but that will come in a later refactor).

Initially I created the ContiguousDigitSeq and DigitSeq abstractions to solve problems with apint allocations. For example if you have a 10000-bit width apint instance and you want to add a 200-bit number to it you need to allocate another 10000-bit instance since apint requires operations on same-width instances. By using digit iterators instead of digit slices I would be able to avoid these allocations in many scenarios. However, I never came to using their potential so they remain useless as of now. Also I do not know if this approach would have gained the best possible performance even though avoiding many huge allocations for big numbers.

I will change arithmetic functions that use &[Digit] to rather use DigitSeq so my functions which heavily use indexing can be affected by the flag. Some functions like the basic shift functions will be broken by this but I planned on improving their performance anyways by rewriting them to index form.

As I have said I am really not eager to merge functionality that will allow users to forcefully, intentionally or accidentally invalidate inherent invariants of safe abstractions that are not marked as unsafe. I am faithful that we can find a useful design that solves our performance problems. There are many tech-talks about this topic of how to achieve great performance without sacrificing safety and by sticking to high-level abstractions. So I am also a bit skeptical about using inline assembly since I deeply think that you can nearly always achieve great performance without it. The main goal of apint is having an efficient 100% Rust implementation for arbitrary sized machine integer emulation.

So to summarize, I plan on doing the following, which I don't think is too much for one refactor:
*the reorganization
*the change to DigitSeq
*improving documentation
*maybe also do error improvements

I am very much in favor of your roadmap, however please keep in mind my skeptical points about your ideas to achieve better performance. At first we should concentrate on getting it working correctly. Then we can improve its performance. Correctness is always more important than performance since nobody will use something that only sometimes works.

[AaronKutch]

I will not make a "unchecked_input" flag and functionality because of your concerns and the performance gains would only be noticeable for single Digit ApInts anyway. I will still do the "unchecked_internals" (disabled by default of course) and make a note in the readme that undefined behaviour results with the flag enabled if there is a bug in an algorithm that causes the function to panic with the flag disabled.
Also, I forgot to mention that I will change all Bit instances to booleans along with the DigitSeq changes, but every other construct should be kept.

[AaronKutch]

I am looking at the standard ops implementations for ApInts and should we add the shift ops? Also, some of the implementations have all three for &'b mut ApInt, for &'b ApInt, and for ApInt, while others lack the first one with mut.

[Robbepop]

I will not make a "unchecked_input" flag and functionality because of your concerns and the performance gains would only be noticeable for single Digit ApInts anyway. I will still do the "unchecked_internals" (disabled by default of course) and make a note in the readme that undefined behaviour results with the flag enabled if there is a bug in an algorithm that causes the function to panic with the flag disabled.
Also, I forgot to mention that I will change all Bit instances to booleans along with the DigitSeq changes, but every other construct should be kept.

To be honest, I am very concerned about the crate feature flag that may cause undefined behaviour as I have stated above. I think it is not a very elegant solution for the problem and I really think that especially with Rust we can do a lot better to tackle the efficiency problem.

The Bit to bool change is welcome since it mirrors the standard library more closely.

I am looking at the standard ops implementations for ApInts and should we add the shift ops? Also, some of the implementations have all three for &'b mut ApInt, for &'b ApInt, and for ApInt, while others lack the first one with mut.

Good point! I am not sure if it was by intention to not implement some operations for &mut T. But depending on what the standard lib does we should mirror that for convenience.

As far as I know there are shift operations for apint instances. :)

[AaronKutch]

Should I change digit.rs so that digit::ONES and the like become Digit::ONES? I don't know any reason not to.

I figured out that the new module system does not really eliminate mod.rs but instead what you do is for every subdirectory there is a .rs file with the same name at src/ level which eliminates the multiple mod.rs file problem at least.
I think this is the layout I will use:
conversion (folder)
|-casting (.rs file)
|-constructors
|-rand_impl
|-serde_impl
|-serialization
|-to_primitive
data
|-access
|-apint
|-apint
|-digit_seq
|-digit
|-int
|-storage
|-uint
|-utils
info
|-bitpos
|-bitwidth
|-errors
|-radix
|-shiftamount
|-traits
logic
|-add
|-bitwise
|-div
|-fuzz
|-ll
|-mul
|-relational
|-shift
|-traits
conversion.rs
data.rs
info.rs
lib.rs
logic.rs

Whenever any submodule imports stuff from around the library, I have made it look like this for example:

use data::{ApInt, Storage, Digit};
use info::{BitWidth, Width, Error, Result};

To be clear, the public structs and functions the user of the library sees should still look the same as before the internal reorganization though.

Folder conversion turned out to be almost completely made out of impl ApInt, and utils.rs has the try_forward_bin_mut_impl stuff which doesn't quite fit in with "data" but those are the only things I am dissatisfied about. What are your thoughts?

[Robbepop]

Okay this is going to be a lot bigger than I initially thought.
Could we please go one step back and reiterate on this?

And also we should try to make PRs smaller in general.
So we could go like this:

1. Convert the entire crate to 2018 edition which is already a large enough work item for one PR.
2. Discuss why we want a new crate structure - we are both okay with having a new crate structure but we have not decided yet upon a concrete one. Your presented structure is a major overhaul of the current one which will completely reset everybody who was working on the project so far (basically me).

I have several questions:

1. Why split add and mul and other arithmetic operations in to different modules? Is it worth the hassle?
2. Why throw everything into the conversion folder? This doesn't make sense in my opinion as conversion is just meant to converse between bit widths. Only the casting module in there makes sense to me.
3. I see a reason to have real name module entry points instead of mod.rs all over the place for IDE reasons. However, maybe we can do this as a first step instead of all the other things together?
4. I fail to see how the new structure with conversion, info, data etc. is superior to the current one. Please elaborate on this more.
5. Why separate data (now in info module) away from their logical parts? To be honest this would certainly confuse me. For me this should belong together.

My proposal:

apint
  | - lib.rs
  | - internal.rs
  | - int.rs
  | - uint.rs
internal
  | - arithmetic
     | - add.rs
     | - sub.rs (or include into add.rs)
     | - mul.rs
     | - div_rem.rs
  | - optional
     | - rand_impl.rs
     | - serde_impl.rs
  | - low_level
     | - digit.rs
     | - logic.rs
     | - digit_seq.rs
  | - arithmetic.rs
  | - bitwise.rs
  | - cmp.rs (formerly known as relational.rs)
  | - shift_rotate.rs (incubates shift and rotate functions)
  | - utilities.rs
  | - conversion.rs (formerly known as casting.rs)
  | - instantiate.rs (formerly known as constructors.rs)
  | - rand_impl.rs
  | - to_primitive.rs
  | - optional.rs

[AaronKutch]

It initially took me days worth of working around apint to get a good feel about where everything is located.
We are both familiar with the current organization now, but one of the points of this reorganization is so it is somewhat easier for us to get around and much easier for any future maintainers to get around for the first time.

Why split add and mul and other arithmetic operations in to different modules? Is it worth the hassle?

arithmetic.rs was getting much too long for my liking. What I am going for here is to move most addition related stuff to add.rs (including subtraction, overflowing addition, and the increment ops), multiplication stuff to mul.rs (just one function for now), division stuff to div.rs (a lot of stuff). Addition and subtraction share a lot of stuff, but for integers multiplication and division are very different, hence why there is no sub.rs.

Why separate data (now in info module) away from their logical parts? To be honest this would certainly confuse me. For me this should belong together.

Why throw everything into the conversion folder? This doesn't make sense in my opinion as conversion is just meant to converse between bit widths. Only the casting module in there makes sense to me.

You see, if I organized the crate by what uses what, we would end up with basically our current organization with a lot of files in one folder, that all need each other but are unrelated in function.

One reason is that files like error.rs use a lot of files and are used by many files at the same time.

The main reason for this is there are 14 files which have impl ApInt in them but they can be roughly grouped by the basic ApInt struct and definition (which I grouped together with other allocation structs in data), the constructors and casting and non math or logical intensive conversions (conversion which I should rename), the math and logic (logic), and finally all the support structs I have put in info.

I think what I am naming the modules is confusing you and I could do better. Maybe I should rename conversion to construction, info to something else (but I can't think of anything more accurate and brief), data to structs, and logic to math.

[AaronKutch]

Also, can I make verify_bit_access a method function of BitPos and verify_shift_amount a method function of ShiftAmount?

[AaronKutch]

I was planning on making a note in the readme that says this library considers it a bug to be able induce a panic internally, with the exception of standard library operation impls. I just discovered that there is one more way to induce a panic without doing it on the user side, and that is a From<usize> impl on BitWidth. I checked for uses of it in the library, but the only use of it is done such that it will not panic and returns an option instead. Do we want this thing to continue to be public?

[AaronKutch]

The original verify_bit_access had where P: Into<BitPos> as part of its signature but I realized all uses of it already convert to BitPos before calling the function, so I made the signature
pub(crate) fn verify_bit_access<T>(self, a: &T) -> Result<()> where T: Width and refactored appropriately, likewise for verify_shift_amount.

[AaronKutch]

The crate is compiling again and in the new edition! I have looked over every part of the code and am preparing Clippy and Rustfmt. I am going through the documentation and clearing up potential confusion. I promise not to do more than this in one single commit.

[AaronKutch]

The edition change made some of the places where ====================== is in the documentation break so I removed some of those those. The docs are auto-wrapping around the screen now which is really nice.
A new warning is appearing in from_radix_digits but I will let you handle that and differentiating signed and unsigned string conversion functions after I submit a pull request

[Robbepop]

Hey,
thank you for updating me in this issue.
However, for a better process of this huge undertaking we should discuss this in a PR so I can constantly review changes.

I really do not like having these super giantic PRs and that's why I proposed splitting up this work into several smaller work items but I guess it is already too late for that. So please file the PR (no worries, I won't merge it until you are okay with it) and we can discuss your changes in more detail. That would help a lot!

Thank you for all the work!

[AaronKutch]

For some reason, I have a false memory that you released a new version of apint right after the arithmetic and utilities rewrite (specifically, I thought that the 0.2.0 version had that rewrite in it), which is why I made that pull request to fix the multiplication bug (that I assumed was in the released version and needed to be fixed quickly) and asked you to make a point release. I don't know how it is possible that it took me several months to notice that 0.2.0 was released before I even started working on apint.

[Robbepop]

Yeah we didn't have a release in a long time and we should actually have one in the next weeks with all those changes. We didn't release something since we weren't sure if all the new functionality is stable enough (which it wasn't as you have found out).

[AaronKutch]

Just so you know, I have spring break next week which means a lot of free time for me to work on apint with. I would prefer if I could get stuff reviewed faster that week, so that I don't end up with the situation I had earlier this year where I had a local branch many commits stacked deep. It gets ugly trying to fix commits at the bottom of the stack, and merging those changes up.

Now that I think of it, I could actually take a break from changes that impact the crate as a whole and work on fixing string deserialization and single function stuff that is trivial to merge. So if you can't keep up with the PRs next week, I will just work on single function stuff instead.

[Robbepop]

I can understand you and I will give my best.
However, please note that it is always about both of us.
If you deliver easily-reviewable PRs I can review them quickly.
A PR that is easily reviewable is doing a single thing only. For example it is okay if it streches over the entire code base if I know it is just simple Rust formatting. However, if you are working on complex stuff like algorithms you really should file it on a per-algorithm basis in the best case.

This sounds like good recommendations: https://hackernoon.com/the-art-of-pull-requests-6f0f099850f9

[AaronKutch]

Those are some good recommendations. I can totally split up the upcoming PRs easily. There is one part of unchecked_internals that is going to need ~1000 changed lines, but the rest should all be under 300 lines or be simple stuff. Also, I have difficulty communicating friendliness and emotions through text so just assume that I am friendly almost all the time.

[Robbepop]

Looking forward to your new style of pull requests. ;)

[AaronKutch]

I went through the trouble of using a different Rust toolchain on a different computer (which has a intel processor so I can compare differences between that and AMD), disabling antivirus, customizing MSYS2 to set the internal PATH variable correctly, and finally managed to get the rug crate (which uses GMP) compiling. The performance numbers look good for us so far. We might have to do something special before we ever add a bench folder to the crate, but that is in the far future.

[AaronKutch]

I can't bear the old crate structure, we need rustfmt, and we need to upgrade to Rust 2018. The problem is that all of these are lightly related to each other, which is the primary reason #36 was so large. Should I PR rustfmt stuff first?

[Robbepop]

I can't bear the old crate structure, we need rustfmt, and we need to upgrade to Rust 2018. The problem is that all of these are lightly related to each other, which is the primary reason #36 was so large. Should I PR rustfmt stuff first?

Best to shortly wait for my update.

I can't bear the old crate structure, we need rustfmt, and we need to upgrade to Rust 2018. The problem is that all of these are lightly related to each other, which is the primary reason #36 was so large. Should I PR rustfmt stuff first?

They are not connected with each other so they do not have to live in the same PR and could have been easily shipped with different PRs each. So we could have had these updates long time ago while other (more research heavy) updates could have been in the pipeline in the form of a PR. That's why I always try to promote small PRs.

Btw.: I have found tons of clippy warnings in your old codes (especially in the arithmetics module). I think it would be a good PR to clean up all of them with the help of clippy. Imo clippy is a great tool and teacher.

[AaronKutch]

I would not even bother with trying to clippy the mess that is the old arithmetic.rs until it has been split up and I have reintroduced the last few commits I made to PR #36 that reduces the cognitive complexity of the multiplication and division functions some. If anything should be directly copied from #36, it should be the logic folder which has very few dependencies besides the need for Rust 2018. edit: actually, I just remembered the changes I made to DataAccess and friends, I'm not sure what to start with here

[Robbepop]

I would not even bother with trying to clippy the mess that is the old arithmetic.rs until it has been split up and I have reintroduced the last few commits I made to PR #36 that reduces the cognitive complexity of the multiplication and division functions some. If anything should be directly copied from #36, it should be the logic folder which has very few dependencies besides the need for Rust 2018. edit: actually, I just remembered the changes I made to DataAccess and friends, I'm not sure what to start with here

one by one. first try to find the things that can live without introducing other changes. if parts of your code depend on those new data access snippets then you should first introduce them in isolation for example. And just then introduce the things that build upon them ... again one by one.

[AaronKutch]

I am almost finished with my fall semester at university and will be able to work on apint. Are you ready to accept PRs quickly?

[Robbepop]

I am almost finished with my fall semester at university and will be able to work on apint. Are you ready to accept PRs quickly?

As always: Depends on the PR.
I can always accept small PRs quickly if they adhere to the single responsibility princible - so if they are doing one and only one thing.
For huge PRs that combine many different refactorings I will either not accept them since too complex to review correctly or it will take a longer time.
So many small, single responsibility PRs are faster reviewed than one big PR.

[AaronKutch]

My next PR I plan to do the following easy commits that will fix pain points for every future PR.

• Move digit::ONES into Digit::ONES
• Rename the Width trait file to width.rs to avoid name collision
• one by one bring the standard ops implementations from around the library into one file traits.rs
• Keep the BitWidth::from() impl but remove the BitWidth::new() impl which is the only fallible impl (outside of the standard ops) to not return a Result.
• Remove the BitWidth::from() impl in favor of BitWidth::try_from() to fix Use TryFrom traits for utility types such as BitWidth #41 (might want to have this as its own PR)
• For brevity in internal tests, should I add a bw!() macro which creates and unwraps BitWidths? The different syntax highlighting and bang should make it visible enough to be an exception. Should we make it public?

[Robbepop]

For brevity in internal tests, should I add a bw!() macro which creates and unwraps BitWidths? The different syntax highlighting and bang should make it visible enough to be an exception. Should we make it public?

No please keep it private at first.