What is wrong with my version of strchr?

Question

My assignment is to write my own version of strchr, yet it doesn't seem to work. Any advice would be much appreciated. Here it is:

char *strchr (const char *s, int c) //we are looking for c on the string s
{

    int dog; //This is the index on the string, initialized as 0
    dog = 0;
    int point; //this is the pointer to the location given by the index
    point = &s[dog];
    while ((s[dog] != c) && (s[dog] != '\0')) { //it keeps adding to dog until it stumbles upon either c or '\0'
            dog++;
            }
    if (s[dog]==c) {
            return point; //at this point, if this value is equal to c it returns the pointer to that location
            }
    else {
            return NULL; //if not, this means that c is not on the string
            }
}

Well, one problem is that you name a variable `dog` for no particular reason. A second seems to be that you mix pointers and integers. — Fred Foo, Jul 08 '12 at 21:06
`point` is not a pointer type, so it cannot hold a pointer. This code should not even compile. Moreover, `int` is not an appropriate type for an offset in a string. — R.. GitHub STOP HELPING ICE, Jul 08 '12 at 21:25
@larsmans: Maybe it's to be analogous to `cat` in OP's implementation of `strcat`...? :-) — R.. GitHub STOP HELPING ICE, Jul 08 '12 at 21:27

Jari · Answer 1 · 2012-07-08T21:22:40.413

4

You return "point" which was originally initialized to beginning of string and not moved since then. You don't need that variable at all, but could simply return &s[dog] (although I would prefer something more descriptive than dog as a variable name).

In fact you would survive with something as simple as this:

while (*s != c && *s)
    ++s;

return (*s == c) ? s : NULL;

edited Jul 08 '12 at 21:22

answered Jul 08 '12 at 21:08

Jari

2,152
13
20

1

+1 The `if` in the return part could be replaced with a conditional operator for even better compactness. – Sergey Kalinichenko Jul 08 '12 at 21:20
+1, though there is a cast missing from this implementation (see my comment to @wabepper's answer). – Fred Foo Jul 08 '12 at 21:53

score 4 · Answer 2 · answered Jul 08 '12 at 21:16

4

You are trying to store an address into point but it's an int-variable. You should do something like this:

char *strchr(char *s, char c) {
    int pos = 0;
    while (s[pos] != c && s[pos] != '\0')
        pos++;
    if (s[pos] == c)
        return &s[pos];
    else
        return NULL;
}

By the way: s should be char * not const char * because your returning an pointer to achar, and that wouldn't be a good style ;) (or return const char *)

answered Jul 08 '12 at 21:16

qwertz

14,614
10
34
46

1

The C standard mandates that `strchr` takes `const` pointer but returns a mutable one into the same buffer. So a compliant pure-C implementation has to cast the `const` away. – Fred Foo Jul 08 '12 at 21:51
You can also increment the pointer directly like this: http://pastebin.ubuntu.com/1081902/ – con-f-use Jul 08 '12 at 22:05
You should also use type `size_t` for `pos` rather than `int`. – caf Jul 09 '12 at 00:44

score 0 · Answer 3 · answered Jul 08 '12 at 21:06

0

int point;

This is not the declaration of a pointer, here is how to declare a pointer to an int:

int *bla;

In your case &s[dog] is a pointer to a const char, so you want to declare point this way:

 const char *point;

And as others pointed out, you are actually ignoring this pointer in your function afterwards.

answered Jul 08 '12 at 21:06

ouah

142,963
15
272
331

2

That's true, but the OP doesn't actually want an `int` pointer. – Fred Foo Jul 08 '12 at 21:07

mathematician1975 · Answer 4 · 2012-07-08T21:27:09.167

Here in your code

int point; //this is the pointer to the location given by the index
point = &s[dog];

you are trying to convert a pointer to char to an int, when

char* point = &s[dog];

is what you want. You should have seen this from the return type of your function. You want to return a char* but you return an int (your variable point) or NULL. As you never actually change point, you are actually returning the address of the first character in your array, so your code is not working as you would like anyway. If you insist on this you would be better using

char* point = &s[dog];
while ((*point != c) && (*point != '\0')) {          
   ++point;
}
return (*point == c) ? point : NULL;

But here it looks like you still have a conceptual problem as you want to compare a char with an int. You should work out if you need an int array or a char array. If you want a char array, change your input argument c to be of type char.

What is wrong with my version of strchr?

4 Answers4