3 ms·

I find the has_ghosting() function more problematic and reminds me of why C just sucks even if it is the appropriate choice. c and c2 loop variables so a typo

by bsder 5y ago

I find the has_ghosting() function more problematic and reminds me of why C just sucks even if it is the appropriate choice. c and c2 loop variables so a typo can hose you horribly. A global variable to hold the array length. ! instead of comparison to 0. Having to offset the second loop by 1. Early return which means the function will normally work fine but might result in N^2 extra time depending upon the data state. A bit trick relying on unsigned underflow without pointing out that unsigned is a key constraint even though it has a comment. An extra missing const on the incoming pointer (should be: const uint8_t * const). Braces left off the short-circuit if conditionals. The worst part is I have personally written tons of functions like this. This is a "normal" function in C--in fact, it's far better than average. The fact that you write C like this just shows how much we need something better.

account42 5y ago
> A global variable to hold the array length. This is a microcontroller firmware. Moving what is essentially a constant around the stack would be insanity. > ! instead of comparison to 0 Not a problem and even idiomatic. > Early return which means the function will normally work fine but might result in N^2 extra time depending upon the data state. It only takes N^2 time when all columns have at least one key pressed but not have at least two rows in common in any of them (except maybe the last two) - hardly a state that you care that much about. Would you really want to increase the latency/power consumption for single key presses just to have the exceptional case not be slower than normal? If you wanted to you could restrict the early exit with the same test for having at least two bits set. If the key matrix has no unused slots you would then only check if there is another column which shares any set row bits (because it would then share all of them), which could let you make the algorithm O(n) with some additional memory for row bit counts. But the code would be more complicated and the number of columns is not dymanic. Plus the key matrix most likely has unused slots precisely to avoid ghosting for common combinations. > A bit trick relying on unsigned underflow without pointing out that unsigned is a key constraint even though it has a comment. The variable with type is declared right before the bit trick. If you change it to signed when there are bit operations, especially ones you don't understand, then you deserve what you get. However, this trick does not rely on unsigned underflow. the only concern with signed (assuming two's complement) would be if the sign bit is the only one set, in which case the signed underflow would be undefined at the language level - but not a problem at the hardware level since two's complement addition/subtraction is exactly the same as unsigned addition/subtraction. > An extra missing const on the incoming pointer (should be: const uint8_t * const). const on the outside of function parameter types (as opposed to inside them) does not change how the function can be called. Sure, you could make all local variables const if they can be but for such a tiny function that really does not add anything. > Braces left off the short-circuit if conditionals. Meh, that is a code style choice and no reason to complain about C. Also not really that dangerous with modern compilers that warn based on misleading indentation. And you missed the actually bad part of the function: The comment that says the colummns are ORed together when they are ANDed.

by bsder 5y ago

account42 5y ago

> A global variable to hold the array length. This is a microcontroller firmware. Moving what is essentially a constant around the stack would be insanity. > ! instead of comparison to 0 Not a problem and even idiomatic. > Early return which means the function will normally work fine but might result in N^2 extra time depending upon the data state. It only takes N^2 time when all columns have at least one key pressed but not have at least two rows in common in any of them (except maybe the last two) - hardly a state that you care that much about. Would you really want to increase the latency/power consumption for single key presses just to have the exceptional case not be slower than normal? If you wanted to you could restrict the early exit with the same test for having at least two bits set. If the key matrix has no unused slots you would then only check if there is another column which shares any set row bits (because it would then share all of them), which could let you make the algorithm O(n) with some additional memory for row bit counts. But the code would be more complicated and the number of columns is not dymanic. Plus the key matrix most likely has unused slots precisely to avoid ghosting for common combinations. > A bit trick relying on unsigned underflow without pointing out that unsigned is a key constraint even though it has a comment. The variable with type is declared right before the bit trick. If you change it to signed when there are bit operations, especially ones you don't understand, then you deserve what you get. However, this trick does not rely on unsigned underflow. the only concern with signed (assuming two's complement) would be if the sign bit is the only one set, in which case the signed underflow would be undefined at the language level - but not a problem at the hardware level since two's complement addition/subtraction is exactly the same as unsigned addition/subtraction. > An extra missing const on the incoming pointer (should be: const uint8_t * const). const on the outside of function parameter types (as opposed to inside them) does not change how the function can be called. Sure, you could make all local variables const if they can be but for such a tiny function that really does not add anything. > Braces left off the short-circuit if conditionals. Meh, that is a code style choice and no reason to complain about C. Also not really that dangerous with modern compilers that warn based on misleading indentation. And you missed the actually bad part of the function: The comment that says the colummns are ORed together when they are ANDed.