3
\$\begingroup\$

I'm trying to write a function that will work out the length of two string char arrays, using pointer arithmetic. It seems to be working but I'm still getting used to pointers and addresses etc and would be grateful if you could let me know what you think. If possible could tell me if my understanding is correct in thinking that, when finding the length of a string char array, you're actually finding how many bytes it is? Thank you.

Here is the code:

#include <stdio.h>
#include <stdlib.h>

int cmpstr(const char *,const char *);

int main()
{

    const char hello[10] = "hello";
    const char world[10] = "world";

    printf("%d", cmpstr(hello, world));

    return 0;
}

int cmpstr(const char *s1, const char *s2)
{
    int start1 = s1; // start address for s1 string

    while(*s1 != '\0')
    {
        s1++;        // increase address until the end
    }

    s1 = s1-start1;  // subtract start address from end address


    int start2 = s2; // start address for s1 string

    while(*s2 != '\0')
    {
        s2++;       // increase address until the end
    }

    s2 = s2-start2; // subtract start address from end address


    if(s1==s2)      // if s1 string is equal to s2 string
    {
        return 0;
    }
    else if(s1>s2)  // if s1 string is greater than s2 string
    {
        return 1;
    }
    else if(s1<s2)  // if s1 string is less than s2 string
    {
        return -1;
    }

}
\$\endgroup\$
5
\$\begingroup\$

type of difference between pointers

pointer_one - pointer_two

The result of this expression is a ptrdiff_t (§6.5.6/9). Assigning it back to a pointer converts an integer type to a pointer, which IIRC should give you a compiler warning as it can lead to all kind of trouble.

relational operators with pointers

Comparing pointers with relative operators is undefined behavior if the pointers don't point into the same array (§6.5.8/5).

else if(s1>s2) // undefined behavior
{
  return 1;
}
else if(s1<s2) // undefined behavior
{
  return -1;
} 

naming

cmpstr isn't really a suitable name for a function that compares the lengths of two strings.

comments

// if s1 string is equal to s2 string

You don't compare strings for equality. You (try to) compare their lengths.

Also, your comments should explain why you wrote the code the way you did. Don't tell what the code is doing. With well written code that should be self apparent.

do one thing per function

cmpstr calculates the length of two strings (code duplication!) and then compares these. That's too much. Delegate that length calculation to another function: strlen. Or - for learning purpose only - write your own replacement.

ptrdiff_t vs size_t

Wondered why strlen returns a size_t instead of a ptrdiff_t? That's because a string might be longer than the maximum difference that can be represented by ptrdiff_t. Thus end - start may actually be undefined behavior when done with really long strings. Better keep a size_t counter that you increase while searching for the '\0'.

program output should end with a '\n'

This is only minor, but otherwise it may corrupt the controlling terminal a bit. Or like in ideone drop the output completely.

printf("%d\n", cmpstr(hello, world));
//        ^^

test your code

You really should! Every (in an ideal world) piece of code that you write should be tested. You might want to look into TDD (test driven development) and build up good habits from the start.

making it faster

Do you really need to obtain the length of both strings? Take these two strings as example:

""
"the quick brown fox did something but I don't remember what"

When you start at the first char of each of those, at how many chars do you need to look to determine which string is longer?

\$\endgroup\$
1
\$\begingroup\$

The first thing compare code attempts is to assign a pointer to an int.

An int may not save all the information inform a pointer. Example: a 64-bit pointer saved to a 32-bit int certainly loses information.

int cmpstr(const char *s1, const char *s2)
{
    int start1 = s1; // start address for s1 string

If code needs to save the value of s1, simple use a variable of the same type.

    // int start1 = s1;
    const char * start1 = s1;

Various weak coding practices are rapidly detected when the compiler warnings are fully enabled.

  int start1 = s1; // start address for s1 string
  // warning: initialization makes integer from pointer without a cast [-Wint-conversion]

}
//  warning: control reaches end of non-void function [-Wreturn-type]

As code ends without an obvious return, better to insure a final return.

  // if (s1 == s2) {
  //   return 0;
  // } else if (s1 > s2) {
  //   return 1;
  // } else if (s1 < s2) {
  // return -1;
  // }
  if (s1 == s2) {
    return 0;
  }
  if (s1 > s2) {
    return 1;
  }
  return -1;

when finding the length of a string char array, you're actually finding how many bytes it is?

Consider comparing the lengths of the 2 strings as code marches down them. No need to compute nor save the length of the strings.

int cmpstr_length(const char *s1, const char *s2) {
  while (*s1 && *s2) {
    s1++;
    s2++;
  }
  if (*s2) {
    return -1;
  }
  if (*s1) {
    return 1;
  }
  return 0;
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.