Re: [PATCH] soc_camera: match signedness of soc_camera_limit_side()

["Németh Márton" <nm127@freemail.hu>]

[-- Attachment #1: Type: text/plain, Size: 1810 bytes --]

Guennadi Liakhovetski wrote:
> You didn't reply to my most important objection:
> 
> On Wed, 27 Jan 2010, Németh Márton wrote:
> 
>> diff -r 31eaa9423f98 linux/include/media/soc_camera.h
>> --- a/linux/include/media/soc_camera.h	Mon Jan 25 15:04:15 2010 -0200
>> +++ b/linux/include/media/soc_camera.h	Wed Jan 27 20:49:57 2010 +0100
>> @@ -264,9 +264,8 @@
>>  		common_flags;
>>  }
>>
>> -static inline void soc_camera_limit_side(unsigned int *start,
>> -		unsigned int *length, unsigned int start_min,
>> -		unsigned int length_min, unsigned int length_max)
>> +static inline void soc_camera_limit_side(int *start, int *length,
>> +		int start_min, int length_min, int length_max)
>>  {
>>  	if (*length < length_min)
>>  		*length = length_min;
> 
> I still do not believe this function will work equally well with signed 
> parameters, as it works with unsigned ones.

I implemented some test cases to find out whether the
soc_camera_limit_side() works correctly or not. My biggest problem is that I'm
not sure what is the expected working of the soc_camera_limit_side() function.

Nevertheless I tried expect some values, you can probably verify whether my
expectations are correct or not (see the test attached).

The signed and unsigned version of the function works differently because
the unsigned version cannot accept negative values. These values will be
implicitly casted to an unsigned value which means that they will be interpreted
as a big positive value.

Here are the test results:

Test Case 1: PASSED
Test Case 2: PASSED
Test Case 3: FAILED: start=50, length=8, start_unsigned=0, length_unsigned=1600
Test Case 4: PASSED
Test Case 5: PASSED
Test Case 6: PASSED
Test Case 7: PASSED
Test Case 8: PASSED

There is a difference in case 3, but which is the correct one?

Regard,

	Márton Németh

[-- Attachment #2: test.c --]
[-- Type: text/x-csrc, Size: 4920 bytes --]


#include <stdio.h>

static inline void soc_camera_limit_side_UNSIGNED(unsigned int *start, unsigned int *length,
		unsigned int start_min, unsigned int length_min, unsigned int length_max)
{
	if (*length < length_min)
		*length = length_min;
	else if (*length > length_max)
		*length = length_max;

	if (*start < start_min)
		*start = start_min;
	else if (*start > start_min + length_max - *length)
		*start = start_min + length_max - *length;
}


static inline void soc_camera_limit_side(int *start, int *length,
		int start_min, int length_min, int length_max)
{
	if (*length < length_min)
		*length = length_min;
	else if (*length > length_max)
		*length = length_max;

	if (*start < start_min)
		*start = start_min;
	else if (*start > start_min + length_max - *length)
		*start = start_min + length_max - *length;
}


int main() {
	int start, length;
	unsigned int start_unsigned, length_unsigned;

	printf("Test Case 1: ");
	start = 0;
	length = 8;
	start_unsigned = start;
	length_unsigned = length;
	soc_camera_limit_side(&start, &length, 0, 8, 1600);
	soc_camera_limit_side_UNSIGNED(&start_unsigned, &length_unsigned, 0, 8, 1600);
	if (start == 0 && length == 8 && start_unsigned == start && length_unsigned == length) {
		printf("PASSED\n");
	} else {
		printf("FAILED: start=%i, length=%i, start_unsigned=%i, length_unsigned=%i\n", start, length, start_unsigned, length_unsigned);
	}

	printf("Test Case 2: ");
	start = -5;
	length = 1600;
	start_unsigned = start;
	length_unsigned = length;
	soc_camera_limit_side(&start, &length, 0, 8, 1600);
	soc_camera_limit_side_UNSIGNED(&start_unsigned, &length_unsigned, 0, 8, 1600);
	if (start == 0 && length == 1600 && start_unsigned == start && length_unsigned == length) {
		printf("PASSED\n");
	} else {
		printf("FAILED: start=%i, length=%i, start_unsigned=%i, length_unsigned=%i\n", start, length, start_unsigned, length_unsigned);
	}

	printf("Test Case 3: ");
	start = 50;
	length = -15;
	start_unsigned = start;
	length_unsigned = length;
	soc_camera_limit_side(&start, &length, 0, 8, 1600);
	soc_camera_limit_side_UNSIGNED(&start_unsigned, &length_unsigned, 0, 8, 1600);
	if (start == 50 && length == 8 && start_unsigned == start && length_unsigned == length) {
		printf("PASSED\n");
	} else {
		printf("FAILED: start=%i, length=%i, start_unsigned=%i, length_unsigned=%i\n", start, length, start_unsigned, length_unsigned);
	}

	printf("Test Case 4: ");
	start = 500;
	length = 2000;
	start_unsigned = start;
	length_unsigned = length;
	soc_camera_limit_side(&start, &length, 0, 8, 1600);
	soc_camera_limit_side_UNSIGNED(&start_unsigned, &length_unsigned, 0, 8, 1600);
	if (start == 0 && length == 1600 && start_unsigned == start && length_unsigned == length) {
		printf("PASSED\n");
	} else {
		printf("FAILED: start=%i, length=%i, start_unsigned=%i, length_unsigned=%i\n", start, length, start_unsigned, length_unsigned);
	}

	printf("Test Case 5: ");
	start = -20;
	length = 1600;
	start_unsigned = start;
	length_unsigned = length;
	soc_camera_limit_side(&start, &length, 100, 8, 1600);
	soc_camera_limit_side_UNSIGNED(&start_unsigned, &length_unsigned, 100, 8, 1600);
	if (start == 100 && length == 1600 && start_unsigned == start && length_unsigned == length) {
		printf("PASSED\n");
	} else {
		printf("FAILED: start=%i, length=%i, start_unsigned=%i, length_unsigned=%i\n", start, length, start_unsigned, length_unsigned);
	}

	printf("Test Case 6: ");
	start = 1600;
	length = 1600;
	start_unsigned = start;
	length_unsigned = length;
	soc_camera_limit_side(&start, &length, 100, 8, 1600);
	soc_camera_limit_side_UNSIGNED(&start_unsigned, &length_unsigned, 100, 8, 1600);
	if (start == 100 && length == 1600 && start_unsigned == start && length_unsigned == length) {
		printf("PASSED\n");
	} else {
		printf("FAILED: start=%i, length=%i, start_unsigned=%i, length_unsigned=%i\n", start, length, start_unsigned, length_unsigned);
	}

	printf("Test Case 7: ");
	start = 5;
	length = 300;
	start_unsigned = start;
	length_unsigned = length;
	soc_camera_limit_side(&start, &length, 100, 8, 1600);
	soc_camera_limit_side_UNSIGNED(&start_unsigned, &length_unsigned, 100, 8, 1600);
	if (start == 100 && length == 300 && start_unsigned == start && length_unsigned == length) {
		printf("PASSED\n");
	} else {
		printf("FAILED: start=%i, length=%i, start_unsigned=%i, length_unsigned=%i\n", start, length, start_unsigned, length_unsigned);
	}

	printf("Test Case 8: ");
	start = 200;
	length = 4;
	start_unsigned = start;
	length_unsigned = length;
	soc_camera_limit_side(&start, &length, 100, 8, 1600);
	soc_camera_limit_side_UNSIGNED(&start_unsigned, &length_unsigned, 100, 8, 1600);
	if (start == 200 && length == 8 && start_unsigned == start && length_unsigned == length) {
		printf("PASSED\n");
	} else {
		printf("FAILED: start=%i, length=%i, start_unsigned=%i, length_unsigned=%i\n", start, length, start_unsigned, length_unsigned);
	}

	return 0;
}