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

replace tweetNaCL exercise with Qoi interface #138

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

squell
Copy link
Member

@squell squell commented Dec 20, 2024

This is a backport from rust-training material.

@hdoordt
Copy link
Member

hdoordt commented Dec 20, 2024

Great work on the exercise! Looks really good.

I don't see why this needs to replace the TweetNaCl exercise, though. It's nice for people to be able to pick more exercises


The QOI C library is a header-only library, which means the function implementations are included within the header file instead of in a separate C file. We've added a separate C file which includes the header to make it easier to compile and include the library in our Rust program.

### Generating bindings
Copy link
Member

Choose a reason for hiding this comment

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

Please use the #[modmod:exercise_ref] to enable modmod to number the exercises upon generating. Example

Copy link
Member Author

Choose a reason for hiding this comment

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

Would it be possible to hack modmod so it just 'sees' that or any markdown subheading? Or is there use case why we would want to have unnumbered subheadings?

@squell
Copy link
Member Author

squell commented Dec 23, 2024

Great work on the exercise! Looks really good.

I don't see why this needs to replace the TweetNaCl exercise, though. It's nice for people to be able to pick more exercises

I mean sure if you want, we can keep it, but the tweetnacl exercise has some didactic problems:

  • There's a extern declaration for a randombytes() in tweetnacl.c that's not provided. On Linux/MacOS that turns out to not be an issue if you don't reference the functions that call it, but on Windows we've seen people run into problems.

  • tweetNaCL is actually a very poor example of a C library. If people think, well that's nice, let's also implement crypto_hash_sha256_tweet you're off into the deep end (you notice there's not a literal definition of crypto_hash_shaXYZ_tweet. If you persevere you see that you can make it work by editing tweetnacl.h in exactly the right way---but then you will loose the implementation for crypto_hash_sha512_tweet. In tweetNaCL, a lot of macro trickery was involved to squeeze it in 100 tweets and this is the downside.

  • There's not really a tangible way for students to feel that the bindings are actually doing something useful since by definition a SHA512 will just be a collection of 64 meaningless bytes. With Qoi it's pretty rewarding: you get an image.

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.

2 participants