Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31544 )
Change subject: commonlib: Add Bubble sort algorithm ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/#/c/31544/2/src/commonlib/sort.c File src/commonlib/sort.c:
https://review.coreboot.org/#/c/31544/2/src/commonlib/sort.c@18 PS2, Line 18: static void swap(int *v1, int *v2) Should this maybe be a macro (using typeof()) in some header to be more widely useful?
https://review.coreboot.org/#/c/31544/2/src/commonlib/sort.c@26 PS2, Line 26: static int is_bigger(int v1, int v2) This... seems a bit overkill?
https://review.coreboot.org/#/c/31544/2/src/commonlib/sort.c@51 PS2, Line 51: idx = i + 1; I don't get the point of the idx variable. This is updated on every loop iteration but doesn't actually affect the loop. Why don't you just say len = i at the end (or len-- which would be equivalent)?
Really, this whole function seems to be a very unobvious way to write
for (j = 0; j < num_entries - 1; j++) for (i = 0; i < num_entries - 1 - j; i++)
https://review.coreboot.org/#/c/31544/2/src/commonlib/sort.c@54 PS2, Line 54: } while (len > 1); You should include an early abort case for when no swaps were made during a whole outer loop iteration. If we're using bubblesort, we should at least make sure we can benefit from one of its biggest advantages (O(n) on close-to-already-sorted data).