teach-rs icon indicating copy to clipboard operation
teach-rs copied to clipboard

add replacement for tweetNaCL exercise

Open squell opened this issue 11 months ago • 6 comments

This is a backport from rust-training material.

squell avatar Dec 20 '24 15:12 squell

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

hdoordt avatar Dec 20 '24 18:12 hdoordt

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.

squell avatar Dec 23 '24 16:12 squell

@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

hdoordt avatar Jan 21 '25 13:01 hdoordt

That sounds reasonable (you're an inclusionist I see, I'm probably more a deletionist?).

squell avatar Jan 30 '25 10:01 squell

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 avatar Jan 30 '25 10:01 squell

@squell Looks good at first glance. You can always try generating a track containing this exercise and see if it came out well

hdoordt avatar Jan 31 '25 07:01 hdoordt

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 safety comments for all unsafe blocks? 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()) }
    }
}

tdittr avatar Jul 18 '25 09:07 tdittr