memcmp C implementation - any logical errors with this one

Question

memcmp C implementation - any logical errors with this one?

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.

Answer 1

Look at your for loop. It's only examining one character.

Answer 2

Strictly speaking the signature is wrong. The correct one is:

int memcmp(const void *s1, const void *s2, size_t n);

Your code compares c and k and on finding that c is less than k dutifully returns a -1. However, if these two were equal you would get an incorrect result since you are returning early.

If you read the documentation you'd find:

The sign of a non-zero return value shall be determined by the sign of the difference between the values of the first pair of bytes (both interpreted as type unsigned char) that differ in the objects being compared

Which basically means you are doing the right thing by returning something that preserves the sign of ('c' - 'k').

A simpler implementation can be found here.

Answer 3

I guess since char signedness is implementation defined, you could make your comparison unsigned:

int memcmp_test(const char *cs_in, const char *ct_in, size_t n)
{
  size_t i;  
  const unsigned char * cs = (const unsigned char*) cs_in;
  const unsigned char * ct = (const unsigned char*) ct_in;

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

Answer 4

As it's written now, this code will only test the first byte of the inputs. The else return 0 needs to be moved out of the loop, leaving return 0 at the end:

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

Answer 5

The return 0; occurs after only comparing the first character. It should be placed outside of the loop.