AaronKutch commented 5 years ago

Some things to note:

My problems with spacing where fixed by letting Rustfmt do its thing on the entire crate, and then following up with discarding specific line changes and adding #[rustfmt::skip] wherever I disagreed with Rustfmt.
I will wait on updating the int.rs and uint.rs files to reflect the new functions and docs.
All TODOs are either performance related or will be fixed in upcoming PRs.
The three warnings will be fixed in upcoming PRs.
There is something weird going on with the interaction between rand and cargo doc, but it is probably due to our outdated dependency should be fixed in a future commit. I plan on updating all the dependencies after the upcoming PRs but before 0.3.0 is released.
R.I.P. commit history, but I promise not to do anything this invasive in the future.

Edit: uh-oh, Rustfmt did not fix the spacing between // and the rest of the comment. I will have to add on a commit to fix this

coveralls commented 5 years ago

Coverage increased (+1.6%) to 77.481% when pulling d7ba1a4be4b1649b13ffb8fc06f59326dd0d69d8 on AaronKutch:reorganization_rebase into bc41a187a7415ac4890a26e3287375d5a2eb7795 on Robbepop:master.

coveralls commented 5 years ago

Coverage increased (+1.3%) to 77.224% when pulling cde6d4445f926c5ed652d4accd97e8c8a5d781ec on AaronKutch:reorganization_rebase into bc41a187a7415ac4890a26e3287375d5a2eb7795 on Robbepop:master.

codecov-io commented 5 years ago

Codecov Report

Merging #36 into master will increase coverage by 1.53%. The diff coverage is 89.71%.

@@            Coverage Diff             @@
##           master      #36      +/-   ##
==========================================
+ Coverage   75.94%   77.48%   +1.53%     
==========================================
  Files          22       29       +7     
  Lines        5117     5471     +354     
==========================================
+ Hits         3886     4239     +353     
- Misses       1231     1232       +1

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	`100% <ø> (+100%)`	:arrow_up:
src/data/int.rs	`0% <0%> (ø)`
src/data/uint.rs	`0% <0%> (ø)`
src/logic/shift.rs	`100% <100%> (ø)`
src/construction/constructors.rs	`99.37% <100%> (ø)`
src/info/bitwidth.rs	`98.41% <100%> (ø)`
... and 32 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 bc41a18...d7ba1a4. Read the comment docs.

Robbepop commented 5 years ago

Is this ready again for another review? If so I would try to do that on the upcoming weekend. :)

I am also looking into writing some proper contribution guidelines and CI integration for rustfmt and clippy with a decent lint level to make this project more feasible for contributions from the community.

AaronKutch commented 5 years ago

That should fix most of the problems. This is ready for review. I highly value your feedback.

Robbepop commented 5 years ago

I stopped reviewing this (again) giantic PR because it was just a mess to review all the changes properly if they are mixed with all the unrelevant rustfmt changes. Please entangle those by reverting all the rustfmt related changes and put them in another PR instead. This makes it possible to review this PR in respect of the important reorganizations that happened and also makes reviewing the other PR easier when I know all the changes are only formatting concerns. Thanks!

AaronKutch commented 5 years ago

It is ready to review again. Remember that the comment spacing fixing is going to be part of the rustfmt PR.

AaronKutch commented 5 years ago

We get a lot more unused warnings when default features are disabled. Spamming cfg_attr and allow(unused_ ...) everywhere doesn't seem the right way to go, is there a better way?

AaronKutch commented 5 years ago

I think maybe we could have a cfg_attr that allows unused functions whenever all the features are not turned on. Then, we just need a note in contributions.md or wherever that reminds us to check that this is working properly before every release, to make sure this isn't failing silently. We could have a similar note to make sure that only the right functions are directly using fields in ApInts and ApIntData.

AaronKutch commented 5 years ago

My free time is opening up again. I am preparing some commits to fix some issues you had when reviewing this PR. I noticed during cleaning up inconsistencies with the trait implementations that there is cloning going on in the ones with &'a ApInt. Should I remove these because it hides cloning that is going on (this crate naturally favors explicitness when cloning is involved), and because it would eliminate a bunch of repeated code? For example:

impl<'a> Sub<&'a ApInt> for ApInt {
    type Output = ApInt;

    fn sub(self, rhs: &'a ApInt) -> Self::Output {
        self.into_wrapping_sub(rhs).unwrap()
    }
}

// remove this one
impl<'a, 'b> Sub<&'a ApInt> for &'b ApInt {
    type Output = ApInt;

    fn sub(self, rhs: &'a ApInt) -> Self::Output {
        self.clone().into_wrapping_sub(rhs).unwrap()
    }
}

Robbepop commented 5 years ago

My free time is opening up again. I am preparing some commits to fix some issues you had when reviewing this PR. I noticed during cleaning up inconsistencies with the trait implementations that there is cloning going on in the ones with &'a ApInt. Should I remove these because it hides cloning that is going on (this crate naturally favors explicitness when cloning is involved), and because it would eliminate a bunch of repeated code? For example:
impl<'a> Sub<&'a ApInt> for ApInt {
    type Output = ApInt;

    fn sub(self, rhs: &'a ApInt) -> Self::Output {
        self.into_wrapping_sub(rhs).unwrap()
    }
}

// remove this one
impl<'a, 'b> Sub<&'a ApInt> for &'b ApInt {
    type Output = ApInt;

    fn sub(self, rhs: &'a ApInt) -> Self::Output {
        self.clone().into_wrapping_sub(rhs).unwrap()
    }
}

Sounds good! To your proposed changes. I am not sure but tbh you are right that this is some hidden expensive computation that should ideally not happen. So maybe you are right about removing the second trait impl!

AaronKutch commented 5 years ago

This is ready for another round of review

AaronKutch commented 5 years ago

I can't do the parsing PR before I do the error refactor before I do the rustfmt refactor before this PR is reviewed and accepted. This is blocking all of my work on this crate now, just so you know.

AaronKutch commented 5 years ago

I decided to replace the slice rotate i wrote with the standard library's slice::rotate_right, since I plan on improving the standard library function itself

Robbepop commented 5 years ago

I decided to replace the slice rotate i wrote with the standard library's slice::rotate_right, since I plan on improving the standard library function itself

Sounds like a really good idea! Looking forward to see progress on it.

AaronKutch commented 5 years ago

My performance improvements are almost in the standard library.

Robbepop commented 5 years ago

Cool! Thanks for letting me know. Can you provide a link to the PR?

AaronKutch commented 5 years ago

I have been looking at my multiplication implementation to see if I could improve performance around negative numbers. In the case of the division operations, I could mutate both self and rhs, but for the multiplication function only self could be mutated. There are ways to make the multiplication more efficient, but they are extremely awkward and strictly less performant than being able to mutate rhs (temporarily, but then rhs is restored). Could I change the signature to wrapping_mul_assign(&mut self, rhs: &mut ApInt)? Maybe I could instead make a second function.

Robbepop commented 5 years ago

Maybe I could instead make a second function.

This, and we should only introduce this new function if the wins are actually significant.

AaronKutch commented 5 years ago

The link to the PR is here. It took a lot more work than expected to make an improvement nearly everywhere

AaronKutch commented 5 years ago

A recap on things this PR won't fix:

I will wait a few more PRs before updating the int.rs and uint.rs files to reflect the new functions and docs.
I will run rustfmt as the very next PR
All TODOs are performance related or will be fixed in upcoming PRs
I will probably update the dependencies right after the rustfmt PR, and fix the breaking changes. This is mainly for ease of development (some of these dependencies are really old).
The warnings will be fixed in upcoming PRs
I think clippy has gone through some changes since the last time I fixed its warnings. Don't even bother running clippy on this PR, I have fixed most of those problems.

Robbepop commented 5 years ago

All TODOs are performance related or will be fixed in upcoming PRs

Please, for the sake of clarity create an issue for every TODO comment left so that we can track them and discuss them.

The warnings will be fixed in upcoming PRs

Are they a major problem?

AaronKutch commented 5 years ago

The warnings are all unused warnings or the one rand documentation warning. It isn't worth it to track each TODO, all 15 of them are pretty self-explanatory. I am not bothering with fixing them yet.

AaronKutch commented 5 years ago

Is there someone you know who could help review this?

AaronKutch commented 5 years ago

Would you rather me close this PR and start over from square one? I fear though that it would take the rest of this summer to get the state of this crate to where this PR is.

Robbepop commented 5 years ago

I think the main problem with PRs like this is that they are just too big to be properly reviewed so that the reviewer actually get the grasp of the changes. That's why normally you try to encapsulate only one or at most a few connected changes in one PR and keep it small to make reasoning about the changes simple.

Programming is hard enough after all and it is easy enough to get something wrong.

Even though the state of this PR is not review friendly I am leaning in on merging it as soon as I am faithful in my review and understanding about it since you put so much work and energy into it. I wouldn't want to waste all that work.

Since I do not know a single other person that could or would review this PR let's do the following: I will give it another review this weekend and if things are not too bad will simply merge it. Otherwise I come back to you and we decide what we do.

AaronKutch commented 5 years ago

I have figured out a way to fix our problems. What I have done is start back to master before this PR, use git mv to move src to old_src and paste in a new_src that has what this PR wants to end up as. I do not commit anything in the new_src besides mostly empty files. What I then do is selectively paste source code from the old_src to the new_src and delete the corresponding part of old_src. I then commit a commit that only moves the code. Then, I Ctrl+Z in my editor so that the new_src returns to what it was before, and commit an update commit in the new_src that shows what actually changed at a line-by-line level from the old_src. Eventually, there is nothing left in old_src which I delete and I use git mv again to end up with a single src directory in the desired state.

It should be vastly easier for you to review, I should finish it today.

AaronKutch commented 5 years ago

I was almost done when I made a mistake and tried to undo a commit that involved deleting a file, which in my experience leads to weird stuff happening like Github desktop showing a different status than my git bash. I added on two more commits at the very end to fix the undo artifacts. The bottom line is that everything is passing without errors, in both cargo test --release and cargo build.

AaronKutch commented 5 years ago

Ok, I built the crate on a different computer and got a new error that I presume came from a rare breaking change. I updated my nightly again, and now more stuff is breaking in the dependencies. I am going to try to rebuild with updated dependencies, without changing much more than the Cargo.toml

AaronKutch commented 5 years ago

nope, its just a compiler bug https://github.com/rust-lang/rust/issues/62562

AaronKutch commented 5 years ago

It still won't work on the nightly-07-09, so I am just going to wait for a new nightly.

AaronKutch commented 5 years ago

It works on stable

Robbepop commented 5 years ago

It works on stable

Would be better if it worked on nightly as well but stable is clearly more important! We should add nothing to the crate that makes it depending on nightly Rust again. Compatibility with stable Rust is considered a feature.

AaronKutch commented 5 years ago

I am rebasing one more time to improve

AaronKutch commented 5 years ago

It is working on the latest nightly and stable now. This is ready for review. The only question I have, is should I add commits onto the end or use rebasing and amending when fixing problems that you point out?

Robbepop commented 5 years ago

It is working on the latest nightly and stable now. This is ready for review. The only question I have, is should I add commits onto the end or use rebasing and amending when fixing problems that you point out?

Just add commits I will sqash-merge it anyway. Tbh I was hoping for far less than 5k (or 10k dep. how you look at it) given that the most changes are simple moves. I will give the PR a review today (sunday) and decide how we continue from here.

AaronKutch commented 5 years ago

What time zone do you live in? I live in UTC -6

AaronKutch commented 5 years ago

I am actually not sure when I should click the "Resolve conversation" button.

Robbepop commented 5 years ago

I am actually not sure when I should click the "Resolve conversation" button.

It is generally the job of the person that opened it so the person can hereby say if they are satisfied with the outcome of the answer. TL;DR: You shouldn't unless it is something trivial such as a quickfix.

AaronKutch commented 5 years ago

When you clone my branch and run cargo doc --no-deps, is there anything that looks wrong?

I am going to be so glad when this finally gets merged and squashed. After this, we do some Rustfmt rounds, I run updated Clippy, and then we can finally get to business with smaller PRs.

AaronKutch commented 5 years ago

Is there more you can get done this weekend?

AaronKutch commented 5 years ago

My slice rotation code just got merged in rust PR #61937!

AaronKutch commented 5 years ago

Every few weeks I get tripped up by excess_bits, thinking that it returns the number of unused bits. You have some documentation saying "Note: A better name for this method has yet to be found!". I didn't want to rename it odd_bits because of name collision with odd numbers. After looking up synonyms, I think the best thing I can rename it is weird_bits. Can I rename it this? I also want to add an unused_bits function which does the opposite and does not use an Option.

AaronKutch commented 5 years ago

I made a overflowing multiplication (not public currently) for the string functions I am working on, and in the process broke up the multiplication into more manageable chunks.

AaronKutch commented 5 years ago

I rushed that commit whoops

AaronKutch commented 5 years ago

I was moving into my college dorm room and wanted to do one last commit before I left. Can you merge this PR before next week?

Robbepop commented 5 years ago

I was moving into my college dorm room and wanted to do one last commit before I left. Can you merge this PR before next week?

I am at CCCamp from on tomorrow so won't have that much time this week.

AaronKutch commented 5 years ago

I am suddenly in need of a bunch of overflowing_ and checked_ operations. I am just going to have to wait for this PR to be accepted, it is too much to add on.

AaronKutch commented 5 years ago

div.rs is still about 1400 newlines long, but that is due to extra whitespace. I reduced cognitive complexity quite a bit by splitting up the division algorithm into its 3 main parts, and removing a really complex unroll. I will still need the constructors overhaul to remove the macros and improve it more.

Robbepop commented 5 years ago

Hey, sorry for my late response to your latest comment. However, I do not feel like this PR is ever going to be merged in its current bloaty state. I am really sorry to say that because of all the work you have put into this PR (and others). I am willing to accept your changes but they have to be completely reorganized into many smaller PRs (this single PR would easily suffice to be split into 10 smaller PRs or so). Then we can simply review them in chunks and everything will be easy to review.

Your other option is that you create a fork of this library with your changes so you can apply every change without my review. Of course this would be a disrution in the ecosystem if we have two similar crates instead of focussing efforts on a single one. The license, however, happily allows you to do so as well.

Please do not feel rejected by my decision. Your work has been great and really appreciated by me. However, this PR went kind of sideways by simply trying to solve all problems at once.

Robbepop / apint

Crate reorganization and upgrade to Rust 2018 #36

Codecov Report