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;
}
}
2 Answers 2
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 char
s do you need to look to determine which string is longer?
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;
}