Skip to content
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

Crate reorganization and upgrade to Rust 2018 #36

Closed
wants to merge 86 commits into from

Conversation

AaronKutch
Copy link
Contributor

@AaronKutch AaronKutch commented Feb 23, 2019

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
Copy link

coveralls commented Feb 23, 2019

Coverage Status

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

@coveralls
Copy link

Coverage Status

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

@codecov-io
Copy link

codecov-io commented Feb 23, 2019

Codecov Report

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

Impacted file tree graph

@@            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%) ⬆️
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
Copy link
Owner

Robbepop commented Feb 26, 2019

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
Copy link
Contributor Author

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

rustfmt.toml Outdated
@@ -0,0 +1,24 @@
# all formatting is executed and checked on the developer's side
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be very happy if you use the following rustfmt.toml file configuration and re-format the entire PR: https://github.com/Robbepop/pdsl/blob/master/.rustfmt.toml

This format config is well suited for reviewing PRs.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I am thinking about adding the following extra compiler warnings and lints to improve the readability of the entire code base so that it becomes easier for users not familiar with the code base to hack on. After your changes and the new release we might see more users.

https://github.com/Robbepop/pdsl/blob/master/model/src/lib.rs#L19

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides that I am planning to extend the CI pipeline to include rustfmt checks as well as clippy lints.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it would be awesome if you could already check for clippy warnings in your PR if you didn't do that already. ;)

mod to_primitive;

pub(crate) use self::to_primitive::PrimitiveTy;
mod casting;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This git diff looks strange to me. What has changed here? Is it a preview bug?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think what is happening here is that the newlines were \r\n and all converted to \n. Next time I will make the newline conversion a separate commit

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay thanks for clearing that up!

where
W: Into<BitWidth>,
{
where W: Into<BitWidth> {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This where clause formatting is inferior to the previous one when it comes to easily edit a where clause which happens frequently during prototyping of new code. I am really a fan of the old formatting because of this reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@@ -198,8 +191,8 @@ impl ApInt {
if target_width < actual_width {
return Error::extension_bitwidth_too_small(target_width, actual_width)
.with_annotation(format!(
"Cannot sign-extend bit-width of {:?} to {:?} bits. \
Do you mean to truncate the instance instead?",
"Cannot sign-extend bit-width of {:?} to {:?} bits. Do you mean to truncate \
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these reformattings of comments the result of rustfmt auto formatting?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Do you not like them?

/// the given `ApInt`
/// - [`sign_extend`](struct.ApInt.html#method.sign_extend)
/// otherwise
/// - [`truncate`](struct.ApInt.html#method.truncate) if `target_width` is less than or equal to
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have the feeling that this PR is only soooo immensely big because you merged reorganization with auto formatting. Please make PRs with a singular step in the future. So this one huge PR would be at least two PRs.

// ApInt::repeat_digit(width_512, 0x0000_FFFF_FFFF_0000),
// ApInt::repeat_digit(width_512, 0x0000_0000_FFFF_FFFF),
// ApInt::all_set(width_512),
/* ApInt::zero(width_256),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am strictly against having C-style comments in any Rust code base.
Please either remove this part completely since it seems to be not relevant since long time or revert the comment change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason, rustfmt wants to do this. I will remove it.

pub fn from_bool(bit: bool) -> ApInt {
ApInt::new_inl(BitWidth::w1(), Digit(bit as DigitRepr))
}
pub fn from_bool(bit: bool) -> ApInt { ApInt::new_inl(BitWidth::w1(), Digit(bit as DigitRepr)) }
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I won't comment the following auto-formatting issues but to be honest I really dislike these single super long lines. I am very much in favor of short lines that make it easy to follow along and review in PRs.

@Robbepop
Copy link
Owner

Robbepop commented Mar 2, 2019

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 AaronKutch force-pushed the reorganization_rebase branch from 488cad8 to b833d82 Compare March 2, 2019 19:31
@AaronKutch
Copy link
Contributor Author

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

mod to_primitive;

pub(crate) use self::to_primitive::PrimitiveTy;
mod casting;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay thanks for clearing that up!

src/construction/casting.rs Show resolved Hide resolved
}
}

//this is put here to be able to constrict the publicity of `ApIntData`
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does that mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The one problem we will have with the reorganization is that I could not make some of the fields of Apint and ApIntData private. I moved some test code (that used private fields) into that file so that I could constrict the visibility as much as I could. Everything except for the len field is constricted to pub(in crate::data), and from there I searched the codebase manually to make sure the fields were only being directly accessed in the right places.

src/data/uint.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
}
zeros
}
}

// ===========================================================================
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where have all these gone?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those went into the traits.rs file along with all the other trait impls for ApInts

///
/// However, this function avoids allocation and has many optimized branches for different input
/// sizes and shifts.
pub fn rotate_left_assign<S>(&mut self, shift_amount: S) -> Result<()>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could these implementations maybe profit from using https://doc.rust-lang.org/std/primitive.u64.html#method.rotate_left and https://doc.rust-lang.org/std/primitive.slice.html#method.rotate_left under the hood? I guess they could a lot!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Funnily enough, I ended up not using any core bitwise rotate functions due to the way this algorithm played out. The single Digit case, for example, did not use it because of support for odd width ApInts.
In the future, I will use the slice rotate function. I am keeping the current one for now because it is faster than the standard one for 1024 bit ApInts or less. I will eventually submit a PR to improve the standard library slice rotate function, and once that is approved I will submit a PR to simplify the apint function a little.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After your work on the standard rotation functions would this be worth a try again?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Digit wise rotation code in question currently uses the standard rotation function, and thus will benefit. The rest of the code deals with bitwise stuff.

@AaronKutch
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

AaronKutch commented May 9, 2019

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
Copy link
Owner

Robbepop commented May 9, 2019

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
Copy link
Contributor Author

This is ready for another round of review

@AaronKutch
Copy link
Contributor Author

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 AaronKutch force-pushed the reorganization_rebase branch from ae49071 to 2b0f493 Compare May 22, 2019 17:08
@AaronKutch
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

My performance improvements are almost in the standard library.

@Robbepop
Copy link
Owner

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

@AaronKutch
Copy link
Contributor Author

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
Copy link
Owner

Maybe I could instead make a second function.

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

@AaronKutch
Copy link
Contributor Author

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

@AaronKutch
Copy link
Contributor Author

AaronKutch commented Jul 2, 2019

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
Copy link
Owner

Robbepop commented Jul 2, 2019

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
Copy link
Contributor Author

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
Copy link
Contributor Author

Is there someone you know who could help review this?

@AaronKutch
Copy link
Contributor Author

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
Copy link
Owner

Robbepop commented Jul 8, 2019

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
Copy link
Contributor Author

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
Copy link
Contributor Author

Is there more you can get done this weekend?

@AaronKutch
Copy link
Contributor Author

AaronKutch commented Aug 9, 2019

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

@AaronKutch
Copy link
Contributor Author

AaronKutch commented Aug 18, 2019

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.

Copy link
Owner

@Robbepop Robbepop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Excess bits state the intent clearer than "weird" or "odd" bits imo.
Just saw that I still had a pending PR from ages ago on this ....

src/data/apint.rs Outdated Show resolved Hide resolved
@AaronKutch
Copy link
Contributor Author

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
Copy link
Contributor Author

I rushed that commit whoops

@AaronKutch AaronKutch force-pushed the reorganization_rebase branch from cdb3f58 to f029095 Compare August 20, 2019 16:42
@AaronKutch
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Owner

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 Robbepop closed this Sep 22, 2019
@AaronKutch
Copy link
Contributor Author

I don't understand. I know this crate inside out by now, and I thought we were really close to merging this. Of course something as big as this is going to have problems, but I have looked over everything multiple times and you have double checked and revealed some small issues which I fixed. I can't think of any problems with what this PR in its current state would do to master, except for the needed rustfmt and pasting a bunch of stuff into uint.rs and int.rs (which is trivial stuff that I can do in two weekends, I have a rustfmt.toml ready to go with suggestions from the last time I tried to rustfmt everything). This PR plus the rustfmt and uint.rs stuff brings the crate back to a state that is strictly better than the current master in every way. I researched the TODOs and do not see any user-facing problems that did not exist beforehand. The minimum that is needed for a 0.3.0 from that point is adding on the new little and big endian constructors (I already found the function signatures I need, most of the changes from this will be in tests that use multi digit constructors), the brand new serialization functions (almost ready to go, I would not make my floating-point serialization function public of course, it needs a bunch of bike-shedding but could otherwise be used internally). After that, I cannot think of any breaking changes that users of the library could detect, except for the error refactor which will only be noticeable if someone is looking into the error struct.

This PR brings stuff like the overflowing functions which I have not made public yet out of an abundance of caution. My conservative estimate is that we could put out a 0.3.0 by the end of the year, and close every one of the issues open now by 2020. If you commit to closing this PR, my conservative estimate regarding the rate at which you accept PRs is that I will be graduated and have some kind of job that consumes all my time before this crate ever gets close to the 0.3.x I envision, and development will be softlocked worse than it already is.

@Robbepop
Copy link
Owner

Robbepop commented Sep 22, 2019

Your work is not lost.
The PR will not be reopened. My decision is final in this regard.
However, I am happy to see this being split up into several many smaller PRs but that is upon yours decide.

I envision, and development will be softlocked worse than it already is.

I see it quite contrary to this. To be honest the big PRs that touch nearly every bit of a codebase pretty much drained all my motivation to work on other stuff for the crate because everything is going to change anyway and touching everything also conflicts with everything which is a mess. Maybe that is a personal or subjective opinion but that's how it felt to me always.

I will now work on some small things that are moving the crate into a better shape for contributions.
Please reconsider splitting your PR up into smaller ones where each is focused on a single thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants