memcmp implementation - any logical errors with this one

[kaku_lala]

Hi Guys, First post here, so be gentle :-)

I was going looking for an implementation of memcmp(), I found this code snippet, but it is clearly marked that there is 1 logical error with the code snippet. Could you help me find the logical error.

Basically, I tested this code against the string.h library implementation of memcmp() with different inputs, but the expected output is always the same as the library version of the function.

Here is the code snippet:

#include <stdio.h>
#include <string.h>

int memcmp_test(const char *cs, const char *ct, size_t n)
{
  size_t i;   

  for (i = 0; i < n; i++, cs++, ct++)
  {
    if (*cs < *ct)
    {
      return -1;
    }
    else if (*cs > *ct)
    {
      return 1;
    }
    else
    {
      return 0;
    }
  }
} 



int main()
{
	int ret_val = 20; //initialize with non-zero value 

	char *string1 = "china";
	char *string2 = "korea";

	ret_val = memcmp_test(string1,string2,5); 

	printf ("ret_val is = %d",ret_val);

	getchar();
	return 0;
}

I ran the program with the two example strings and the program would return just after the comparison of the first characters of the two strings. ret_val is -1 in the above case.

The definition of memcmp() which the above code snippet should conform to is:

The definition of the ‘C’ Library function memcmp is
int memcmp(const char *cs, const char *ct, size_t n)

Compare the first n characters of cs with the first n characters of ct.
Return < 0 if cs < ct.
Return > 0 if cs > ct.
Return 0 if cs == ct.

There is definitely on LOGICAL error, could you help me find it.

[John A]

else
    {
      return 0;
    }

Is a logic error because you assume that if the first byte of each block are equal, the entire block of memory is the equal. "return 0" should be placed after the for() loop, because if there hasn't been an invalid comparison, then clearly the blocks of memory are equal.

[ArkM]

If you want to follow C libary function memcmp specification (why not? ), take into account that:

The sign of a nonzero value returned by the comparison functions memcmp, strcmp, and strncmp is determined by the sign of the difference between the values of the first pair of characters (both interpreted as unsigned char)...

Now remember memcmp prototype:

int memcmp(const void*,const void*,size_t);

So should be:

if (*(unsigned char*)cs < *(unsugned char*)ct) { ...

and so on...

[Dave Sinkula]

Bear in mind some implementations describe themselves rather than the standard.

The memcmp function returns an integer greater than, equal to, or less than zero, accordingly as the object pointed to by s1 is greater than, equal to, or less than the object pointed to by s2.

The strcmp function returns an integer greater than, equal to, or less than zero, accordingly as the string pointed to by s1 is greater than, equal to, or less than the string pointed to by s2.

[Ancient Dragon]

>>int memcmp_test(const char *cs, const char *ct, size_t n)
I think the first two parameters should be const unsigned char because memcmp() can work with binary blocks of data and typcasting unsigned char to char might produce undefined behavior.

[twomers]

I'd've thought if you were comparing binary data the memcmp function would have taken const void* 's rather than any kind of char* abstraction.

[ArkM]

>typcasting unsigned char to char might produce undefined behavior.
May be unexpected result but never undefined behaviour...

The three types char, signed char, and unsigned char are collectively called the character types. The implementation shall define char to have the same range, representation, and behavior as either signed char or unsigned char.

[kaku_lala]

Thanks everyone!!

From your answers, I gather that apart from the syntactical error (const void*'s rather than char* and const unsigned char), the logical error with the code is that "return 0" should be out of the for loop.

Thanks very much.

[ArkM]

>...the syntactical error (const void*'s rather than char* and const unsigned char)...
It's not a syntactical error but pragmatics and incomplete design issues...