0

I am trying to write code to implement strchr function in c. But, I'm not able to return the string.

I have seen discussions on how to return string but I'm not getting desired output

const char* stchr(const char *,char);
int main()
{
    char *string[50],*p;
    char ch;
    printf("Enter a sentence\n");
    gets(string);
    printf("Enter the character from which sentence should be printed\n");
    scanf("%c",&ch);
    p=stchr(string,ch);
    printf("\nThe sentence from %c is %s",ch,p);
}
const char* stchr(const char *string,char ch)
{
    int i=0,count=0;
    while(string[i]!='\0'&&count==0)
    {
        if(string[i++]==ch)
            count++;
    }
    if(count!=0)
    {
        char *temp[50];
        int size=(strlen(string)-i+1);
        strncpy(temp,string+i-1,size);
        temp[strlen(temp)+1]='\0';
        printf("%s",temp);
       return (char*)temp;
    }
    else
        return 0;
}

I should get the output similar to strchr function but output is as follows

Enter a sentence
i love cooking
Enter the character from which sentence should be printed
l
The sentence from l is (null)
Federico klez Culloca
  • 26,308
  • 17
  • 56
  • 95
Hema Latha
  • 25
  • 3
  • 2
    Use a debugger to step through the code. It will show you where your logic is wrong. – Paul Ogilvie Oct 28 '19 at 10:34
  • 2
    Learning how to debug is an important part of learning how to program. – Paul Ogilvie Oct 28 '19 at 10:34
  • 2
    Why are you doing all those things in the `stchr` function? All that `strchr` does it finding the `char` in question and then returning the address of that `char`. No need for this suspect dynamic memory routine. Unless your `strch` shouldn't actually be a `strchr` implementation and is supposed to do something else that you didn't mention. – Blaze Oct 28 '19 at 10:37
  • 3
    1) `string`, in main function is actually an array of char pointers (not a string). `char string[50];` is the correct declaration. 2) as someone mentioned above, your `strchr()` has a strange funtionallity: it counts the number of `ch` occurrencies in string and returns a brand new string instead of providing the address of `ch` first occurrency. 3) If you want a function returning a copy of the original string, you cant return the address of a local variable. It's contained in thaa stack, and it will be overwritten after the function return. – Roberto Caboni Oct 28 '19 at 10:47
  • Don't use gets: https://stackoverflow.com/questions/1694036/why-is-the-gets-function-so-dangerous-that-it-should-not-be-used – William Pursell Oct 28 '19 at 11:21
  • Except from the fact that `gets` should never be used, this code has TONS of warnings. Read them. They show what's wrong. – klutt Oct 28 '19 at 11:25
  • Why are you copying the string? You just want to return a pointer to the correct location in the extant string. eg `const char* stchr(const char *s,char ch) { while(*s && *s != ch) s+=1; return *s ? s : NULL; }` – William Pursell Oct 28 '19 at 11:29
  • Here is an implementation of strchr: https://clc-wiki.net/wiki/C_standard_library:string.h:strchr – klutt Oct 28 '19 at 11:34
  • @WilliamPursell You should cast `s` upon return to avoid a warning. `return *s ? (char*)s : NULL;` – klutt Oct 28 '19 at 11:35
  • `strncpy(temp,string+i-1,size);` This does not make any sense. `size` is taken as length of the remaining string. Same what would be done by normal `strcpy` If you want to limit string to your buffer size, you should use the size of your destination buffer as limit. – Gerhardh Oct 28 '19 at 12:06
  • I thought that `strchr` would just return the entire string from matched character... Now, I have realized that's not the case. It only returns the address of the first occurrence of character. Thanks! – Hema Latha Nov 01 '19 at 11:12

1 Answers1

1

There are basically only two real errors in your code, plus one line that, IMHO, should certainly be changed. Here are the errors, with the solutions:

(1) As noted in the comments, the line:

char *string[50],*p;

is declaring string as an array of 50 character pointers, whereas you just want an array of 50 characters. Use this, instead:

char string[50], *p;

(2) There are two problems with the line: char *temp[50]; First, as noted in (1), your are declaring an array of character pointers, not an array of characters. Second, as this is a locally-defined ('automatic') variable, it will be deleted when the function exits, so your p variable in main will point to some memory that has been deleted. To fix this, you can declare the (local) variable as static, which means it will remain fixed in memory (but see the added footnote on the use of static variables):

static char temp[50];

Lastly, again as mentioned in the comments, you should not be using the gets function, as this is now obsolete (although some compilers still support it). Instead, you should use the fgets function, and use stdin as the 'source file':

fgets(string, 49, stdin);/// gets()  has been removed! Here, 2nd argument is max length.

Another minor issue is your use of the strlen and strncpy functions. The former actually returns a value of type size_t (always an unsigned integral type) not int (always signed); the latter uses such a size_t type as its final argument. So, you should have this line, instead of what you currently have:

size_t size = (strlen(string) - i + 1);

Feel free to ask for further clarification and/or explanation.

EDIT: Potential Problem when using the static Solution

As noted in the comments by Basya, the use of static data can cause issues that can be hard to track down when developing programs that have multiple threads: if two different threads try to access the data at the same time, you will get (at best) a "data race" and, more likely, difficult-to-trace unexpected behaviour. A better way, in such circumstances, is to dynamically allocate memory for the variable from the "heap," using the standard malloc function (defined in <stdlib.h> - be sure to #include this header):

char* temp = malloc(50);

If you use this approach, be sure to release the memory when you're done with it, using the free() function. In your example, this would be at the end of main:

free(p);
Adrian Mole
  • 49,934
  • 160
  • 51
  • 83
  • 1
    note that using a static array means the function is not thread safe. – Basya Oct 28 '19 at 11:42
  • @Basya Noted, but for this 'simple' `int main()` program, that will not be an issue. Of course, using `char *temp = malloc(50);` is safer (and actually better, IMHO), but I try to keep things uncomplicated for early-stage learners of the language. My suggestion will solve (and hopefully explain) the issue. – Adrian Mole Oct 28 '19 at 11:47
  • Fair, but I am concerned that someone might copy this and not realize its implications. – Basya Oct 28 '19 at 11:50
  • Cool. That got you an upvote :-) -- the rest of the answer is good information. – Basya Oct 28 '19 at 12:10
  • I didn't realize that my code has these silly mistakes and errors. Thanks for helping me realizing them... – Hema Latha Nov 01 '19 at 11:09