1. 7

  2. 2

    So I would say that far more helpful than any refactoring or renaming (especially of lsb to least_significant_bit or 1 to FIRST_BIT) would be to split out the implementation of this Galois LFSR into a struct called galois_lfsr and a function called galois_lfsr_step. Maybe even providing some macro for maximum period length N bit tap configurations. A function comment (as suggested by the book “The Practice of Programming” by Brian Kernighan and Rob Pike) stating: // galois_lfsr_step: Step a Galois Linear Feedback Shift Register would be the final cherry on top.

    Something like this:

    struct galois_lfsr {
    	unsigned long state;
    	unsigned long taps;
    #define GALOIS_LFSR_MAX_PERIOD_TAPS_17_BITS 0x12000ul
    // galois_lfsr_step: Step a Galois Linear Feedback Shift Register
    unsigned long galois_lfsr_step(struct galois_lfsr *l)
    	bool lsb;
    	assert(l != NULL);
    	lsb = l->state & 1;
    	l->state >>= 1;
    	if (lsb) l->state ^= l->taps;
    	return l->state;

    Now you have a very specific function, which implements a very specific algorithm, you don’t have to worry about the comment becoming out of sync with the code as it would be quite an impressive level of ineptitude for someone to change this function’s logic rather than replacing it. The name of the function and the contents of the single comment would allow potential readers to quickly find a resource such as https://en.wikipedia.org/wiki/Linear-feedback_shift_register#Galois_LFSRs and would mean that rather than having to worry about explaining that if (lsb) rndval ^= TAPS; is actually xoring the taps with the output bit with zero comments, you can instead allow the reader to find a much more verbose and detailed resource on the topic themselves.

    Now you can also use this code in another function which can focus on conveying the fact that 2^8 is 256 (or the smallest power of 2 which covers all the Y coordinates) and that 2^9 is 512 (or the smallest power of 2 which covers all the X coordinates). e.g.:

    enum {
    	SCREEN_WIDTH = 320,
    	SCREEN_HEIGHT = 200,
    #define ULONG_BITS (sizeof (unsigned long) * CHAR_BIT)
    #define BITMASK(n) (ULONG_MAX >> (ULONG_BITS - (n)))
    // As a sidenote: typedefing the non-pointer type allows you to typecheck implementations of your callback like:
    // fizzle_pixel_func your_impl;
    // void your_impl(unsigned long x, unsigned long y, something /* whoops */ *context) { ... }
    // This typechecking will happen BEFORE you ever write the call to the function.
    // Not the most useful, but not totally worthless either.
    typedef void fizzle_pixel_func(unsigned long x, unsigned long y, void *context);
    void fizzlefade(fizzle_pixel_func *fizzle_pixel, void *context) {
    	static const unsigned long lfsr_start_state = 1;
    	struct galois_lfsr lfsr = {
    	do {
    		unsigned long x, y;
    		x = (lfsr.state & SCREEN_WIDTH_MASK) - 1;
    		y = (lfsr.state >> SCREEN_WIDTH_BITS) & SCREEN_HEIGHT_MASK;
    		if (x < SCREEN_WIDTH && y < SCREEN_HEIGHT)
    			fizzle_pixel(x, y, context);
    	} while(lfsr.state != lfsr_start_state);

    With the above code the only real things requiring explanation / fixing are:

    1. SCREEN_{WIDTH,HEIGHT}_BITS are really log2(SCREEN_{WIDTH,HEIGHT}) except…

    2. SCREEN_WIDTH_BITS does not convey the fact that you actually need the log2 of SCREEN_WIDTH + 1 because…

    3. 17 bit maximum period taps cover all 2^17 values except for 0.

    4. I would avoid the callback approach, this should really be done in a similar style to the galois_lfsr struct and function, that you can actually use this code in a modern game to implement a fizzle effect rather than just having the entire screen fizzle in one frame.

    5. By doing it the non-callback way, you can create an initialiser function which solves problem #1 for you and avoids having to maintain the values manually. Alternatively you could calculate those values at compile time which is not a bad idea since generating a header file should be trivial.

    6. Since division is no longer the most expensive operation in the world, using % and / you can make the code a lot simpler. e.g.

      do {
      	unsigned long x, y, pixel;
      	pixel = lfsr.state - 1;
      	x = pixel % SCREEN_WIDTH;
      	y = pixel / SCREEN_WIDTH;
      	if (y < SCREEN_HEIGHT)
      		fizzle_pixel(x, y, context);
      } while(lfsr.state != lfsr_start_state);