C's printf doesn't print a string (char array) out

Question

My question is basically using printf print a char array.

In some cases, it prints the result out:

int main(int argc, char** argv) {
  char* orig = "@reveals#?the treasure chest#$President Barack H. Obama#";
  printf("The input: %s\n", orig);
  printf("The output: %s\n", reArrange(orig));

  return (EXIT_SUCCESS);
}

sometimes not:

int main(int argc, char** argv) {
  char* orig = "@reveals#?the treasure chest#$President Barack H. Obama#";
  printf("%s\n", reArrange(orig));

  return (EXIT_SUCCESS);
}

Here is the complete code (the main function is included):

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

#define SUBJECT '$'
#define ACTION '@'
#define OBJECT '?'
#define END '#'

char* reArrange(char* orig) {
  int origSize = strlen(orig);

  char subject[origSize], action[origSize], object[origSize];

//int i;
//for(i = 0; i < origSize; i++) {
//  subject[i] = ' ';
//  action[i] = ' ';
//  object[i] = ' ';
//}
  int subjectIndex = 0,   actionIndex = 0,  objectIndex = 0;

  int timesEndCharShowUp = 0;

  char state;
  int i;
  for(i = 0; i < origSize; i++) {
    char ch = orig[i];

    if(ch == SUBJECT) {
      state = SUBJECT;
    }
    else if(ch == ACTION) {
      state = ACTION;
    }
    else if(ch == OBJECT) {
      state = OBJECT;
    }
    else if(ch == END) {
      if(timesEndCharShowUp == 3) {
        break;
      }
      else {
        timesEndCharShowUp++;
      }
    }
    else {
      if(state == SUBJECT) {
        subject[subjectIndex++] = ch;
      }
      else if(state == ACTION) {
        action[actionIndex++] = ch;
      }
      else if(state == OBJECT) {
        object[objectIndex++] = ch;
      }
    }
  }

  subject[subjectIndex] = '\0';
  action[actionIndex] = '\0';
  object[objectIndex] = '\0';

  char rearranged[origSize];
  sprintf(rearranged, "%s %s %s.\0", subject, action, object);
  //printf("%s\n", rearranged);

  orig = rearranged;
  return orig;
}

int main(int argc, char** argv) {
  char* orig = "@reveals#?the treasure chest#$President Barack H. Obama#";
//  printf("The input: %s\n", orig);
//  printf("The output: %s\n", reArrange(orig));
  printf("result: ");
  printf("%s\n", reArrange(orig) );
//fflush(stdout);

  return (EXIT_SUCCESS);
}

Blagovest Buyukliev · Accepted Answer · 2011-07-18 17:52:18Z

5

You are returning a pointer to memory that resides on the stack. rearranged is not available after the enclosing function (reArrange) returns and may contain garbage.

You may want to malloc rearranged or declare it globally.

answered Jul 18, 2011 at 17:52

Blagovest Buyukliev

43.7k14 gold badges96 silver badges135 bronze badges

Sign up to request clarification or add additional context in comments.

8 Comments

Blagovest Buyukliev Over a year ago

@Clifford: static really means global but visible only in the enclosing function.

draw Over a year ago

I see, but why printf("The input: %s\n", orig);printf("The output: %s\n", reArrange(orig));works?

Blagovest Buyukliev Over a year ago

When it works, you are lucky. Learn how the stack works and what is undefined behaviour.

Clifford Over a year ago

@Blagovest: No not at all! Global variables are implicitly static, the converse is not true. static allocation is about the lifetime of a variable, globality on the other hand is about visibility and scope - if it has local scope it is by definition not global. Of course by the time you have returned a non-const pointer to it, any protection obtained from making it local is largely lost. Rudy's solution is the better and more usual one in any case.

Clifford Over a year ago

I cannot believe 6 votes for suggesting the use of a global! Required reading; forget that it refers to embedded systems; the principles apply, err... globally ;)

|

Rudy Velthuis · Accepted Answer · 2011-07-18 18:08:50Z

3

Instead of only returning a char *, make reArrange() accept a buffer to which it can write the result. Now the caller must provide a suitable buffer to your function, and you have no problems with memory management anymore. You merely strncpy() arranged to the buffer. To be sure the buffer is large enough, the user should also provide the size of the buffer, in a 3rd argument:

char *rArrange(char *orig, char *result, int resultSize)
{
    if (!result || resultSize == 0)
        return NULL;

    /* your code here */

    strncpy(result, arranged, resultSize);
    return result;
}

The alternative malloc() to store the result is not very user fiendly (the user must freemem() the buffer but may not be aware of this). Using a static/global buffer is not very thread-safe.

edited Jul 18, 2011 at 18:08

answered Jul 18, 2011 at 18:03

Rudy Velthuis

28.9k5 gold badges50 silver badges97 bronze badges

Comments

Clifford · Accepted Answer · 2011-07-19 19:26:45Z

2

Your function needs two parameters:

char* reArrange(char* orig)

Should be:

char* reArrange(char *rearragned, char* orig) {

// make your magic here!

}

Calling sequence:

char input[SIZE];
char rearrange [SIZE];

// initialize everything! Don't forget to rearrange[0] ='\0\;!!!

rearrange(rearranged, input);

// do you printing....

You should also learn how to use pointers correctly and look up "switch".

edited Jul 19, 2011 at 19:26

Clifford

94.3k14 gold badges93 silver badges176 bronze badges

answered Jul 18, 2011 at 18:27

CPD

4272 silver badges11 bronze badges

1 Comment

Clifford Over a year ago

rearrange[0] needs no initialisation in this case, though you might choose to do so.

Heath Hunnicutt · Accepted Answer · 2011-07-18 17:52:56Z

1

The problem is that your function reArrange returns a pointer to memory that it no longer controls. The address of the rearranged array is returned. After the return occurs, this array effectively no longer exists -- the memory can, and will, be reused.

A quick hack which fixes your mistake is to declare rearranged as static. The long term fix is to learn how C works and code something using malloc() or an equivalent.

answered Jul 18, 2011 at 17:52

Heath Hunnicutt

19.6k4 gold badges43 silver badges62 bronze badges

3 Comments

Clifford Over a year ago

Actually in some cases declaring it static is both adequate and safer; not necessarily a hack. If you allocate it dynamically within reArrange() you are making the caller responsible for its disposal, so the caller would have to know how it was allocated. A better (and the most conventional) solution is as detailed by @Rudy Velthuis, then the caller decides on both allocation and disposal, which may or may not be dynamic. The static declaration would make the function non-re-entrant and therefore not thread-safe - as I said "in some cases...".

Heath Hunnicutt Over a year ago

@Clifford -- "Actually," declaring it static makes the function non-thread-safe. Thus, it's a hack -- especially when recommend to an OP who has no idea what is going on.

Clifford Over a year ago

Yes it makes it non-thread safe; that is exactly what I said, no argument - I did stress "in some cases" twice. In many cases thread safety is not an issue; e.g. when the application is not multi-threaded, or the function is only to be used in a single thread. The potential for a memory leak will exist with your suggestion in both single and multi-threaded applications. As I said the best solution is neither of these, but I'd place static allocation ahead of returning a dynamically allocated block.

Jacob · Accepted Answer · 2011-07-18 17:54:05Z

1

With char rearranged[origSize]; you created a new character array which goes out of scope once reArrange terminates. So during the lifetime of reArrange, rearranged is a pointer which points to something meaningful ; hence, orig = rearranged makes sense.

But once it goes out of scope, reArrange(orig) returns the pointer to rearranged which is a dangling pointer now rearranged does not exist anymore.

answered Jul 18, 2011 at 17:54

Jacob

34.7k15 gold badges116 silver badges168 bronze badges

Comments

A. K. · Accepted Answer · 2011-07-18 22:21:11Z

0

Are you sure that the following section of the code works?

char* reArrange(char* orig) {
  int origSize = strlen(orig);

  char subject[origSize], action[origSize], object[origSize];

i mean origSize has to be a const it can't be a dynamic value. You should use malloc for allocating apace for subject , action and object. Moreover, you might consider a few guidelines:

1 Instead of:

for(i = 0; i < origSize; i++) {
    char ch = orig[i];    
    if(ch == SUBJECT) {
      state = SUBJECT;
}

You can have:

char ch;//declare char ch outside for loop
for(i = 0; i < origSize; i++) {
    ch = orig[i];

    if(ch == SUBJECT) {
      state = SUBJECT;
    }

2 you may like to use a switch statement instead of if, that will make your code look great.

answered Jul 18, 2011 at 22:21

A. K.

39.3k17 gold badges58 silver badges106 bronze badges

2 Comments

draw Over a year ago

yeah, that section works. I learned C last summer, and then I quickly forget most non-intuitive ones after one year when I used Java a lot... Truly, switch is better~~

draw Over a year ago

I'm interested the 1st guideline, what's the reason behind it?

Collectives™ on Stack Overflow

C's printf doesn't print a string (char array) out

6 Answers 6

8 Comments

Comments

1 Comment

3 Comments

Comments

2 Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

6 Answers 6

8 Comments

Comments

1 Comment

3 Comments

Comments

2 Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related