0

I am trying to copy the String an char pointer to another ?

I tried the code given below .compiled successfully but output is blank(no output) .

a) Where I was wrong?where my programming logic getting fail?

b) How can I improve this code to get desired output?

    void main()
    {
     char *p="krishna";
     char *q;

      while(*p!='\0')
      {
       *q++=*p++;
      }
      printf("%s",q);
      getch();
    }
kTiwari
  • 1,488
  • 1
  • 14
  • 21

10 Answers10

3
void main()
{
  char *p="krishna";
  char *q;

  /* When copying, you modify your pointers, so you need another one
  ** to store where your string is starting. */
  char *r;

  /* Until now, q points to nowhere.
  ** You need to allocate the place it should point: */
  r = q = malloc(strlen(p) + 1);
  /* Plus 1 because C string uses one extra character in the end,
  ** the NULL ('\0') character. */

  while(*p!='\0')
  {
   *q++=*p++;
  }

  /* Until now, you've copied every character, except the NULL char at the end.
  ** you must set it in order for it to be a valid string. */
  *q = '\0';

  /* Now q points to the last (NULL) character of the string.
  ** In order to print it, you will need a pointer to the start of the string,
  ** that is why we need r: */
  printf("%s",r);

  /* When you are done using a memory buffer you allocated yourself,
  ** you must free it so it can be reused elsewhere. */
  free(r);

  getch();
}
lvella
  • 12,754
  • 11
  • 54
  • 106
2

a) Where I was wrong?where my programming logic getting fail?

Well, you did several things incorrectly. For one, void main is non-standard; the return type for main should be int.

That being said, the real issue you're looking for has to do with the fact that q is uninitialized, yet you attempt to still copy to memory through it (which is undefined behavior). To correct that, try allocating q, e.g.

char *q = malloc(8);

Note you must then later also take care to free the memory allocated here.

Aside from that, you're forgetting to copy the NUL terminator, too.

*q = 0;

... after your copying loop. You also are printing q after incrementing the pointer, so it will no longer be at the head of the string by the time of your printf call. You should store a copy of the head in another variable. Additionally, be careful using a plain printf without any new-lines, as the stream may be buffered and thus could remain unflushed -- use an explicit fflush(stdout);


b) How can I improve this code to get desired output?

Well, the first most straight-forward improvement I can think of is to use strcpy.

#include <stdio.h>

main() {
  const char p[] = "krishna";
  char q[sizeof p];
  strcpy(q, p);
  puts(q);
  getchar();
  return 0;
}
obataku
  • 29,212
  • 3
  • 44
  • 57
slavemaster
  • 204
  • 1
  • 4
1

You should allocate memory for 'q' to hold the copied string.

Vikdor
  • 23,934
  • 10
  • 61
  • 84
  • initially q would have a garbage value(address in this context) so what my point is the copy can be done in that garbage address.... – kTiwari Aug 27 '12 at 17:34
  • That memory is not allocated to your program, so you cannot just write into it and expect it to be there. – Vikdor Aug 27 '12 at 17:36
1

You're not saying how much memory you want for q, so, you're trying to run over something else's memory. So, use malloc, or, arrays.

Dhaivat Pandya
  • 6,499
  • 4
  • 29
  • 43
1

You need to put a '\0' character on the end of q. You also need to allocate enough memory for it.

#include <string.h>
#include <stdlib.h>

int main()
{
  char *p="krishna";
  int size = strlen(p) + 1;
  char *q = (char *) malloc(size);
  char *qi = q;

  do
  {
   *q++=*p;
  } while(*p++!='\0');

  q = qi; //reset q to the beginning of the string

  printf("%s",q);
  getch();
  return 0;
}

I think this should work

fgp
  • 428
  • 2
  • 6
  • initially q would have a garbage value(address in this context) so what my point is the copy can be done in that garbage address.. – kTiwari Aug 27 '12 at 17:39
  • It would work on most cases, but you don't know what memory address q is pointing to, so it may cause your program to crash if it was pointing to a memory region on which you are not allowed to write. – fgp Aug 27 '12 at 17:48
  • your code is seems to be correct ....but on my machine the output is----krishnaÍ!]^éŽù€üÿt .Œñ.‰6ïº .....i think you forgot to null terminate the the string after the while loop ...we have to put *q='\0' before q points to its initial address. – kTiwari Aug 27 '12 at 18:04
  • True, had forgotten to change the place where p is incremented. – fgp Aug 27 '12 at 18:10
1

You are moving the pointer at the end with for loop. Also, you should be allocating memory for q too.

Try this:

void main()
{
 char *p="krishna";
 char *q = malloc(sizeof(char) * 20);
 char *temp = NULL;

 /* save q */
 temp = q;
  while(*p!='\0')
  {
   *q++=*p++;
  }
  /* reset saved q */
  q=temp;
  printf("%s",q);
  getch();
}
Rohan
  • 52,392
  • 12
  • 90
  • 87
  • initially q would have a garbage value(address in this context) so what my point is the copy can be done in that garbage address... – kTiwari Aug 27 '12 at 17:36
  • This is correct. The only suggestion I would make would be to make sure that q is allocated to a size large enough to contain the string 'p' is pointing to (strlen(p)+1). – Ori Pessach Aug 27 '12 at 17:41
  • @krishnaChandra, that is wrong. You won't know which memory address `q` points to. That memory may or may not be writable or belong to different part of your process. – Rohan Aug 27 '12 at 17:43
  • The format specifier `"%s"` expects a null terminated string, not any pointer to char. Failing to provide one is just as much UB as is using garbage pointer values. – eq- Aug 27 '12 at 18:36
  • @Rohan: check my own answer in the context of what you stated that q must point to a memory location......my own answer works – kTiwari Aug 27 '12 at 19:29
1

a) You move q but don't reset it to the beginning of the string.

As many others have pointed out, you also need the destination of the copy operation be someplace where memory is allocated, on the heap with char *buf = malloc(BUF_SIZE) or on the stack with char buf[BUF_SIZE].

Finally the destination must be zero terminated.

b) Using a dedicated API is normally more readable and better performing that home grown loops.

olovb
  • 2,164
  • 1
  • 17
  • 20
1

In this example you provided, you are not allocating memory for the new array of characters, but instead you are just copying over characters into an unknown buffer. This could cause all sorts of problems (read about buffer overflows and attacks). Furthermore, once you are done copying over the characters into the unknown buffer, this buffer is no longer pointing to the beginning of the string, but instead the end of the string. If your idea is to copy over the character array as a string in C, you can do so with the following code:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
void main()
{
  char *p="krishna";
  char *q = (char*) malloc(sizeof(char) * (strlen(p) + 1));

  strcpy(q, p);

  printf("%s", q);
  getchar();
  free(q);
  return 0;
}

In most scenarios, the extra byte at the end of the array for the NULL character ('\0') is unnecessary, but it is included for correctness as character arrays should be end-delimited by the NULL character.

By using malloc, you allocate (dynamic) memory for the buffer that we are copying the string to. Anytime memory is dynamically allocated, it should also be later freed otherwise, it will continue to be used unnecessarily. In order to free up this use of malloc, the 'free' command is used at the end, before the application returns.

Aaron Fujimoto
  • 321
  • 1
  • 6
  • Your example is really no better in preventing buffer overflows; no amount of superficial casts and useless uses of `sizeof` will help you if you allocate, or instruct to allocate, too little memory for your needs. – eq- Aug 27 '12 at 18:33
  • Is my example in allocating memory not allocating enough memory for our needs? The "superficial casts" may be necessary depending on what you are using to compile the code. Specifically, in Microsoft Visual C++ 2010 , under the default settings it will not compile without the casts. The use of sizeof is most likely not needed, but added to be explicit as to what we are allocating size for. This example shows a very short string, but in the case of a longer string (or less memory provided) it may be enough to overrun useful memory. – Aaron Fujimoto Aug 27 '12 at 20:00
0

One important improvement suggestion, which is missing yet (others have already pointed out the string related problems)

Your declaration of main is not correct. 
The return value should  not be  void  but int
  • why? could you please justify it – kTiwari Aug 27 '12 at 17:53
  • @krishnaChandra, I just left a comment on the question, which is the proper place for it. It might not be the standard itself, but it's easier to link to. In case you're wondering, the author is the creator of C++. – chris Aug 27 '12 at 17:55
  • @krishnaChandra, returning a valid value from main should be a way to return something to the caller of main. This may be system specific, but remember you can fetch the value of the last executed command from the shell and you can take some action based on this value.Hope you understand. – Tanmoy Bandyopadhyay Aug 27 '12 at 18:00
  • i am new to c programming didn't get too much of it – kTiwari Aug 27 '12 at 18:06
  • pl. read the first answer from this Stack Overflow question, http://stackoverflow.com/questions/204476/what-should-main-return-in-c-c – Tanmoy Bandyopadhyay Aug 27 '12 at 18:08
0

as many told me to initialize the value of q to a memory address.I think there is no need to do this.because this code is working properly despite of undefined behavior.

 int main()
 {
 char *p="krishna";
 char *q,*r;
 r=q;

  while(*p!='\0')
  {
   *q++=*p++;
  }
   *q='\0';
    q=r;
  printf("%s",q);
  getch();
  return 0;
 }
kTiwari
  • 1,488
  • 1
  • 14
  • 21
  • It only worked by chance. Try compiling it with optimisations, for me that segfaulted while without optimisations it pretended to work. But it's UB, so whether it segfaults depends on the compiler, OS, phase of moon perhaps, ... – Daniel Fischer Aug 29 '12 at 00:44