Finding max element of an array using two heapallocated arrays and a bubble sort

As seen on reddit: https://github.com/tangxunye/android_vendor_qcom_proprietary/blob/e0666c398903d38e72aeda7042ec2836cd3dba68/mmcamera/mmcamera2/mediacontroller/modules/isp/hw/modules/rolloff/mlro_to_plro/mlro_utils.c#L52
Its from Qualcomm so it must be good code, right?
There are plenty of WTFs to go around here. But I think my favorite part is that the function parameters are described in loving detail (
double *x > input onedimensional array
) but the descriptions of what the functions actually do are either useless or missing entirely. I don't suppose you'd want to mention thatbubblesort
sorts the input in reverse order or anything.

What's worse is that the function returns the absolute value of the element with the largest absolute value, not the largest element. (If the array is { 0, 2, 1 }, it returns 2 and not 1.)
It's still the wrong way to do it, of course. There's no need for either of the malloc calls. (Even if you do it by sorting the malloc'ed array, you don't need the array of indices, since that's just used to find and take the absolute value of the original element with the largest absolute value, but you already have that in the zeroth element of the sorted copy.)
All in all, a mega , but there's also the of assuming anything about the quality of code produced by a large corporation. After all, I once worked at a large corporation whose production codebase included a file called
evildead.c
where all the variables and local functions had names related to Romero's film. Even stripped of that , the code was just awful for many other reasons, notably that it was clearly intended to demonstrate that Real Programmers can write Fortran code in any language, except that it had been written by a bad Real Programmer.

@jnz said in Finding max element of an array using two heapallocated arrays and a bubble sort:
Its from Qualcomm so it must be good code, right?
Good one
Just look at one of the monthly Android security bulletins and check how many of the critical vulnerabilities were found in one of the Qualcomm drivers. Also, since when do hardware companies hire competent developers?

@asdf I don't know, let's ask Realtek for example:
Best viewed at 800x600 with IE 6.0 or Netscape 7.02 or Mozilla Firefox 1.0.6 or higher.
Oh.

@deadfast said in Finding max element of an array using two heapallocated arrays and a bubble sort:
Realtek
I made the mistake of visiting their website once. Never again!

@steve_the_cynic The function is called absmax, so I guess returning the maximum absolute value is deliberate (making 2 the correct return value in your example). Seems like a really edge case, but why not. The implementation is a huge , though, on that we can agree.

@asdf said in Finding max element of an array using two heapallocated arrays and a bubble sort:
@deadfast said in Finding max element of an array using two heapallocated arrays and a bubble sort:
Realtek
I made the mistake of visiting their website once. Never again!
I can see why:
Best viewed at 800x600 with IE 6.0 or Netscape 7.02 or Mozilla Firefox 1.0.6 or higher.

Error mem alloc for l
fuck you too

@jnz said in Finding max element of an array using two heapallocated arrays and a bubble sort:
As seen on reddit: https://github.com/tangxunye/android_vendor_qcom_proprietary/blob/e0666c398903d38e72aeda7042ec2836cd3dba68/mmcamera/mmcamera2/mediacontroller/modules/isp/hw/modules/rolloff/mlro_to_plro/mlro_utils.c#L52
Its from Qualcomm so it must be good code, right?
There are plenty of WTFs to go around here. But I think my favorite part is that the function parameters are described in loving detail (
double *x > input onedimensional array
) but the descriptions of what the functions actually do are either useless or missing entirely. I don't suppose you'd want to mention thatbubblesort
sorts the input in reverse order or anything.Without reading the linked code:
double absmax(double *a, size_t n) { double max = 0; double *z = a + n; while( a < z ) { if( *a > max ) { max = *a; else if( *a > max ) { max = *a; } a++; } return max; }

@pleegwat said in Finding max element of an array using two heapallocated arrays and a bubble sort:
Without reading the linked code:
I'd use a temporary that holds the abs'd version of the value in the array, but would otherwise do pretty much the same. (It's probably also best to set max to 1 or some other reasonable sentinel initially.)

@dkf Yeah, I'd forgot that this function would never return negative on nonempty input, so that'd be sensible when passed an empty list.
And using an intermediate would've given me an opportunity to write
val = fabs(*a++)
and tick some people off.

@pleegwat said in Finding max element of an array using two heapallocated arrays and a bubble sort:
fabs
Fabulous!

@asdf said in Finding max element of an array using two heapallocated arrays and a bubble sort:
Qualcomm drivers
They're the ones responsible for Atheros, right? It's rather difficult to get their stuff orkng on Windows 7 because apparently I have to go to the OEM.

@pleegwat said in Finding max element of an array using two heapallocated arrays and a bubble sort:
@jnz said in Finding max element of an array using two heapallocated arrays and a bubble sort:
As seen on reddit: https://github.com/tangxunye/android_vendor_qcom_proprietary/blob/e0666c398903d38e72aeda7042ec2836cd3dba68/mmcamera/mmcamera2/mediacontroller/modules/isp/hw/modules/rolloff/mlro_to_plro/mlro_utils.c#L52
Its from Qualcomm so it must be good code, right?
There are plenty of WTFs to go around here. But I think my favorite part is that the function parameters are described in loving detail (
double *x > input onedimensional array
) but the descriptions of what the functions actually do are either useless or missing entirely. I don't suppose you'd want to mention thatbubblesort
sorts the input in reverse order or anything.Without reading the linked code:
double absmax(double *a, size_t n) { double max = 0; double *z = a + n; while( a < z ) { if( *a > max ) { max = *a; else if( *a > max ) { max = *a; } a++; } return max; }
First thought: oh wow that's evil
Second thought: wait, else can't be inside an if, it has to be after an if
Third thought: oh, there's an opening brace on the while. It's just a typo.

@tsaukpaetra said in Finding max element of an array using two heapallocated arrays and a bubble sort:
@asdf said in Finding max element of an array using two heapallocated arrays and a bubble sort:
Qualcomm drivers
They're the ones responsible for Atheros, right? It's rather difficult to get their stuff orkng on Windows 7 because apparently I have to go to the OEM.
I remember having issues with them in the early days of running Ubuntu, as my laptop at the time had Atheros wifi and there were no drivers. Although I also had the fun choice of running Ubuntu and have nonworking wifi or running Windows and have nonworking keyboard. Keyboards ceasing to work properly on my laptops being a recurring theme it seems...

@ben_lubar said in Finding max element of an array using two heapallocated arrays and a bubble sort:
@pleegwat said in Finding max element of an array using two heapallocated arrays and a bubble sort:
@jnz said in Finding max element of an array using two heapallocated arrays and a bubble sort:
As seen on reddit: https://github.com/tangxunye/android_vendor_qcom_proprietary/blob/e0666c398903d38e72aeda7042ec2836cd3dba68/mmcamera/mmcamera2/mediacontroller/modules/isp/hw/modules/rolloff/mlro_to_plro/mlro_utils.c#L52
Its from Qualcomm so it must be good code, right?
There are plenty of WTFs to go around here. But I think my favorite part is that the function parameters are described in loving detail (
double *x > input onedimensional array
) but the descriptions of what the functions actually do are either useless or missing entirely. I don't suppose you'd want to mention thatbubblesort
sorts the input in reverse order or anything.Without reading the linked code:
double absmax(double *a, size_t n) { double max = 0; double *z = a + n; while( a < z ) { if( *a > max ) { max = *a; else if( *a > max ) { max = *a; } a++; } return max; }
First thought: oh wow that's evil
Second thought: wait, else can't be inside an if, it has to be after an if
Third thought: oh, there's an opening brace on the while. It's just a typo.
Huh, yeah, I missed a brace. If you want compact and evil:
#define MAX(a,b) ((a)>(b)?(a):(b)) #define ABS(a) ((a)>0?(a):(a)) double absmax(double *a, size_t n) { double max = 1.0; double *z = a + n; do max = MAX(max, ABS(*a)) while (++a<z); return max; }
Though that doesn't work on empty lists.

@pleegwat said in Finding max element of an array using two heapallocated arrays and a bubble sort:
Though that doesn't work on empty lists.
You get a nice sentinel
1.0
from an empty list, which might be acceptable.

@dkf said in Finding max element of an array using two heapallocated arrays and a bubble sort:
@pleegwat said in Finding max element of an array using two heapallocated arrays and a bubble sort:
Though that doesn't work on empty lists.
You get a nice sentinel
1.0
from an empty list, which might be acceptable.No, you get a nice UB from dereferencing
a
. It'sdo while
, notwhile
.

@nedfodder Hmm, missed that.

@khudzlin said in Finding max element of an array using two heapallocated arrays and a bubble sort:
@steve_the_cynic The function is called absmax, so I guess returning the maximum absolute value is deliberate (making 2 the correct return value in your example). Seems like a really edge case, but why not. The implementation is a huge , though, on that we can agree.
I'm sure it is deliberate, but it's also ambiguous. Is it "the absolute value of the max" or the "max of the absolute values". (And could we end up with
absmax
andmaxabs
to cover those two cases? And the subsequent endless fun when people choose the wrong one...)I suppose it could be worse. They could have written it to return the unmodified value of the element with the largest absolute value. Ugh.