0

I'm writing code that inputs a file and reads in each word as a separate char* like this:

char label[8];
char type[5];
char value[6];


while (!input.eof()) {
    input >> label;
    input >> type;
    input >> value;
    storeSymbols(label, type, value);
}

Then I set it to an element of a char* array like so:

void storeSymbols(char* lab, char* type, char* val) {
    labels[symCount] = lab;
    types[symCount] = type;
    values[symCount] = val;
    symCount++;

}

However, when I print out all of the elements of the char* array, all of the elements are the same as the last element retrieved from the file. For example, if there were three labels in the document, defined as "one", "two", and "three" then at the end of the first loop, the array will contain "one", the second time it will contain "two" and "two" and the third time it will contain "three" "three" and "three". It also loops through an extra time, adding the last element to the array another time, making it four "three" elements in the array. Does anyone have any idea why this is happening?

I've also tested this where I convert value to an int and add it to an int array and it works fine.

Patrick Henry
  • 3
  • 3
  • Is the second bit of code also inside the loop from the first bit of code? – Xymostech Mar 11 '13 at 01:56
  • I think we need more code here. – SirPentor Mar 11 '13 at 01:58
  • You'll need to duplicate/copy the strings instead of assigning a pointer to the same buffer over and over (which is what you're doing now). – Michael Burr Mar 11 '13 at 02:00
  • No, the second bit is in a separate method and the three arrays are global variables. Also, what more code would you like? – Patrick Henry Mar 11 '13 at 02:01
  • The snippets shown give the impression you`re iterating over the **complete** input first throwing away any information gathered to later only assign the very last values read. Increasing symCount only **once** from 0 to 1 after you skipped over the whole input. To exactly see whats happening more code is needed. – mikyra Mar 11 '13 at 02:10
  • I updated the code. Let me know if you need anything more. Also, symCount is a global int initially set to 0. – Patrick Henry Mar 11 '13 at 02:20

2 Answers2

0

You're clearly using C++, not C, so use std::string instead of char * to store your labels, and a vector instead of an array to store the labels and such.

Therefore:

std::vector<std::string> labels, types, values;

std::string label, type, value;
input >> label >> type >> value;

labels.push_back(label);
types.push_back(type);
values.push_back(value);

The reason why you're seeing the same output repeatedly is because you are pushing pointers to the same fixed global array, which is repeatedly overwritten by input >> label. std::string avoids this by copying and allocating internal buffers (all done automatically).

nneonneo
  • 171,345
  • 36
  • 312
  • 383
  • The problem is this is an optional exercise for a class I'm taking where the teacher told us that we can only use char* since she wants us to primarily focus on coding in C. Couldn't tell you why, but that's how she wants it. – Patrick Henry Mar 11 '13 at 02:17
  • That's so wrong. Just...wrong. You have C++, and you're being asked to code in C. This is going to make poor C++ programmers who think they have nothing better to use but sticks and stones. I claim you should just skip the assignment and inform the teacher that if she wants you to use C, she should damn well teach C and not C++. – nneonneo Mar 11 '13 at 02:18
  • This is what everyone in the class has been saying. I may just use std::string instead and risk getting points taken off. Probably for the best. – Patrick Henry Mar 11 '13 at 02:27
  • The alternative, in my mind, is to go straight C. Ditch the `iostream` stuff and just write basic C. That'll be educational too, in its own way, and you won't be making some crazy mixed up mess of two languages. – nneonneo Mar 11 '13 at 02:29
  • @PatrickHenry if you are asked to code in C, then you should not be using the `>>` operator for input streams anyway, you will lose points for that. – naxchange Mar 11 '13 at 03:15
  • My professor told us to use the the >> operator. As well as many other C++ techniques. But we have to use char*. Makes no sense. – Patrick Henry Mar 11 '13 at 18:13
0

The loop that is reading the input is just storing each item into the same buffer that was used before. For example, input >> label stores the item read into the same char label[8] buffer each time. So at any moment in time, that label array contains only the most recently read item.

Then when you try to save that information in storeSymbols() you pass in the address of the label array - the same address each time. So each element of the labels array (which I assume is an array of char*) gets the same pointer - they each point to the same buffer that is getting updated.

A simple change that will let you store pointers to distinct items might be to use strdup() to duplicate the strings. If you're not allowed to use strdup() in your assignment, an equivalent can be written as a function in less than 10 lines of code.

If you use strdup(), the strings you're not storing pointers to will be dynamically allocated, so you'll need to free them when you're done using them:

for (i = 0; i < symCount; ++i) {
    free(labels[i]);
    free(types[i]);
    free(values[i]);
}

For your question about looping through an extra time after the last items are read from input, the loop that use to read input uses an antipattern of checking for EOF directly. EOF isn't set until you actually try to perform a read when the input stream is 'empty'. See Why is “while ( !feof (file) )” always wrong? and http://drpaulcarter.com/cs/common-c-errors.php#4.2

Try:

while (input >> label >> type >> value) {
    storeSymbols(label, type, value);
}

Also, make sure that your input buffers are large enough for your data (keep in mind the null terminator). Ideally you'd use a data type that would dynamically expand (like std::string) when reading the input.

Community
  • 1
  • 1
Michael Burr
  • 333,147
  • 50
  • 533
  • 760