teach-rs
teach-rs copied to clipboard
add replacement for tweetNaCL exercise
This is a backport from rust-training material.
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
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
externdeclaration for arandombytes()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_tweetyou're off into the deep end (you notice there's not a literal definition ofcrypto_hash_shaXYZ_tweet. If you persevere you see that you can make it work by editingtweetnacl.hin exactly the right way---but then you will loose the implementation forcrypto_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.
@squell I agree with you that this new exercise is an improvement. The reason that I'd like to keep the old one, is that I'd like for teach-rs to be a library of composable educational material. We can 'bless' certain parts by including them in our predefined tracks, but there's no harm in keeping 'unblessed' material around IMO.
If you can alter this PR such that the tweetnacl exercise is preserved, I'll gladly mash that 'merge' button
That sounds reasonable (you're an inclusionist I see, I'm probably more a deletionist?).
I'm a little unsure if I put the exercise_ref's in the correct spot -- they also weren't there in the tweetnacl exercise to help guide me.
@squell Looks good at first glance. You can always try generating a track containing this exercise and see if it came out well
Review notes:
- Files were missing in topic.toml -> I added them
- I really like the example image, gave a nice sense of achievement when I decoded it.
- Should we add a section on writing
safetycomments for allunsafeblocks? It would be good practice in production code. - Similarly should we let the students implement proper error handling?
- I feel like instead of MyImag we should let them create a "QoiSlice" or similar which only wraps the ptr and derefs to &[u8] like this:
pub struct QoiSlice {
ptr: NonNull<u8>,
desc: qoi_desc,
}
impl QoiSlice {
unsafe fn new(ptr: NonNull<u8>, desc: qoi_desc) -> Self {
Self { ptr, desc }
}
fn len(&self) -> usize {
self.desc.width as usize * self.desc.height as usize * self.desc.channels as usize
}
}
impl Drop for QoiSlice {
fn drop(&mut self) {
let ptr = self.ptr.as_ptr() as *mut c_void;
unsafe { libc::free(ptr) }
}
}
impl Deref for QoiSlice {
type Target = [u8];
fn deref(&self) -> &Self::Target {
unsafe { slice::from_raw_parts(self.ptr.as_ptr(), self.len()) }
}
}