views:

724

answers:

7
+2  Q: 

Basic C Question

I am reading over the KR book, and am a little stuck.

What is wrong with the following?

void getInput(int* output) {
   int c, i;
   for(i=0; (c = getchar()) != '\n'; i++)
     output[i] = c; // printf("%c", c) prints the c value as expected
   output[++i] = '\0';
}

When I run the program it never gets out of the loop and I have to ctrl-c to exit. However if I replace the 5th line with printf("%c", c); it prints out all the input just fine after hitting enter and creating the new line.

Thanks for the help.

+1  A: 

It looks correct to me as written except that you don't need to increment i outside of the loop. The i gets incremented right before the loop exits, thus it is already where you want it.

Make sure that a '\n' is actually making it into c.

Sometimes an '\n' will get thrown away as a delimiter.

jjnguy
A: 

a simple way to risk buffer overflow, because output's size is never passed/checked

PW
I had that in before, but having an issue with the code it got stripped out to make it easier to find the issue.
chris
+6  A: 

What is wrong with the following?

1. void getInput(int* output) {

Why is the input argument an int* when what you want to store in an array of characters? Probably

void getInput(char* output) {

is better.

Also, how do you know that the output pointer is pointing somewhere where you hold enough memory to write the user's input? Maybe you must have the maximum buffer length as an extra parameter to avoid buffer overflow errors as PW pointed out.

5.   output[++i] = '\0';

i has already been incremented an extra time inside the for loop, so you can just do:

output[i] = '\0';

Other than these, the program runs fine and outputs what we input until return.

FWIW, I tested it by calling it like so:

 int main(void)
{
    char o[100];
    getInput(o);
    printf("%s", o);
    return 0;
}
sundar
A: 

Here is the complete program with a couple of updates from your input, but it still won't make it out of the loop. BTW this was exercise 1-24 on pg 34

#include <stdio.h>

#define STACK_SIZE 50
#define MAX_INPUT_SIZE 1000
#define FALSE 0
#define TRUE 1

void getInput();
int validInput();

int main() {
  char* userInput[MAX_INPUT_SIZE];

  getInput(&userInput);

  if (validInput(&userInput) == TRUE)
    printf("Compile complete");
  else
    printf("Error");
}

// Functions
void getInput(char* output) {
  int c, i;
  for(i=0; (c = getchar()) != '\n' && c != EOF && i <= MAX_INPUT_SIZE; i++)
    output[i] = c;
  output[i] = '\0';
}

int validInput(char* input) {
  char stack[STACK_SIZE];
  int c;
  int j;

  for (j=0; (c = input[j]) != '\0'; ) {
    switch(c){
      case '[': case '(': case '{':
        stack[j++] = c;
        break;
      case ']': case ')': case '}':
        if (c == ']' && stack[j] != '[')
          return FALSE;
        else if (c == '}' && stack[j] != '{')
          return FALSE;
        else if (c == ')' && stack[j] != '(')
          return FALSE;

        // decrement the stack's index  
        --j;
        break;
    }
  }

  return TRUE;
}
chris
The markup parser is playing with your code, taking < as a sign of beginning of a html tag... I think putting it within a code block (selecting the code and pressing Ctrl-K) might help, though I'm not sure. Strangely, the behaviour doesn't seem reproducible, at least in my simplistic tests.
sundar
For a start, 'char* userInput[MAX_INPUT_SIZE];' should be 'char userInput[MAX_INPUT_SIZE+1];' - that's two changes, char array instead of char-pointer array and extra space for holding null.Funnily enough, the first problem masks the second as you're allocating 4 times as much memory as you need.
paxdiablo
1800 INFORMATION
Since I was having issues with the formatting I cut some of the code off. I have now posted all the code. It does compile on my machine using gcc 4.0.1
chris
A: 

Have you tried using a debugger? You should step through the code in gdb or visual studio or whatever you are using and see what is going on. You said you were a beginner so maybe you hadn't considered that yet - this is a pretty normal debugging technique to use.

1800 INFORMATION
Unfortunately, I am just writing this up with textmate. I will check out Eclipse later today and use the debugger.
chris
+1  A: 

your last code as posted have 3 errors I can see:

char* userInput[MAX_INPUT_SIZE];

Should be:

char userInput[MAX_INPUT_SIZE+1];

(this was already mentioned by Pax Diablo)

getInput(&userInput);

Should be:

getInput( userInput );

This last error means you passed to getInput an address inside your call stack. you have a memory overwrite. probably one of your calls to getchar() returnes to the wrong address.

Raz
That makes sense, otherwise I am passing the address of the pointer.
chris
A: 

Here is the final working code. I must say I picked up quite a bit from doing this one. Thanks for the help and pointers.

Any suggestions on how things could be done better?

#include <stdio.h>

#define STACK_SIZE 50
#define MAX_INPUT_SIZE 1000
#define FALSE 0
#define TRUE !FALSE

void get_input();
int valid_input();

int main() {
  char user_input[MAX_INPUT_SIZE + 1]; // +1 for the \0

  get_input(user_input);

  if (valid_input(user_input))
    printf("Success\n");
  else
    printf("Error\n");
}

// Functions
void get_input(char* output) {
  int c, i;
  for(i=0; (c = getchar()) != '\n' && c != EOF && i <= MAX_INPUT_SIZE; i++)
    output[i] = c;
  output[i] = '\0';
}

int valid_input(char* input) {
  char stack[STACK_SIZE];
  char c;
  int i = 0;
  int stack_index = -1;

  while ((c = input[i]) != '\0' && i < STACK_SIZE) {
    switch(c){
      case '[': case '(': case '{':
        stack_index++; 
        stack[stack_index] = c;
        break;
      case ']': case ')': case '}':
        if ((c == ']' && stack[stack_index] != '[') ||
            (c == '}' && stack[stack_index] != '{') ||
            (c == ')' && stack[stack_index] != '('))
          return FALSE;

        // decrement the stack's index now that the closing bracket is found  
        stack_index--;
        break;
    }
    i++;
  }

  // stack index should be back where it started
  return (stack_index == -1);
}
chris