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

Update to 2018 #47

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Update to 2018 #47

wants to merge 10 commits into from

Conversation

archer884
Copy link

I noticed some of these libraries are pretty old, and so I set out to update them (and, you know, migrate to the 2018 edition). In so doing, I also realized that the rustdct library kind of changed its API on you, apparently just to make life a little more fun. As such, my most important changes (read: the ones I understand the least) are in dct_2d(), on line 44 of dct.rs. The rest of this is pretty much 100% just me screwing with formatting and implementing advice from cargo clippy.

Anyway, here it is if you want it.

A note: at least after I did this, I did not notice any major difference in performance between dct and non-dct hashes (at least in my tiny use case). Of course, I didn't check to see what difference there was beforehand, and I don't know if the code even works correctly (although it seems to be producing valid hashes for me). There's no telling what, if anything, that indicates. That said, I did completely remove use of transpose from that code path. /shrug

@archer884
Copy link
Author

I've been flipping back and forth between the version that I messed with and the original version and I don't really see any performance difference.

@stalkerg
Copy link

@abonander hey, looks like such a library is a little bit abandoned maybe we should make a fork?

@archer884
Copy link
Author

I've got a fork of the repo with my updates in it that I've been using since I opened this PR, but I've never really thought of publishing it.

@stalkerg
Copy link

@archer884 you can check forks network and you can find many repositories with good improvements. Maybe you can consider publishing it and applying some extra things from other repositories? But I hope @abonander can a make comment on it.

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.

None yet

2 participants