Attention is currently required from: Arthur Heymans, Paul Menzel, Yidi Lin, Yu-Ping Wu.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/78798?usp=email )
Change subject: lib: Add GCD function ......................................................................
Patch Set 2:
(4 comments)
File src/commonlib/bsd/gcd.c:
https://review.coreboot.org/c/coreboot/+/78798/comment/caac2e4a_462afb80 : PS2, Line 6: unsigned long gcd(unsigned long a, unsigned long b)
Change to uint64_t to allow larger values?
Looks like all the use cases we have are 32-bit and on x86 doing this at 64-bit would be expensive, so maybe we should restrict this function to uint32_t for now? (Maybe call it `gcd32()` then to clarify?) I agree that `unsigned long` seems a bad choice, it's better to be clear about type widths.
https://review.coreboot.org/c/coreboot/+/78798/comment/3cea4537_5c9c15c9 : PS2, Line 10: if (a == 0 || b == 0) Shouldn't we just let these cases run into the divide-by-zero? I wouldn't consider asking for the GCD of 0 valid.
https://review.coreboot.org/c/coreboot/+/78798/comment/e4df50fa_8be6c5aa : PS2, Line 12: if (a < b) I think writing this explicitly is unnecessary. If `a < b`, then `c = a % b` means `c = a`, so in the first loop iteration you'll get `a = b`, `b = c (the old a)` and then you're back on track.
File src/commonlib/bsd/include/commonlib/bsd/gcd.h:
https://review.coreboot.org/c/coreboot/+/78798/comment/b139cfc4_e9b2a36d : PS2, Line 3: #ifndef __GCD_H__ nit: Include guards should include the COMMONLIB_BSD somewhere so that there won't be any clashes in case we ever get a src/include/gcd.h as well.
-
Julius Werner (Code Review)