Cannot find the error in C++ code

Question

Is there anything wrong with this code? This is a part of function for reading database data. o is otl_stream object for database output stream. My mentor told me that I have mistake in this code, I am newbie to C++ and can't figure out what is problem... I must use pointers, so please don't tell me to use static char array.

char* temp_char;

while (!o.eof()) {

   temp_char = new char [31];
   o>>temp_char;

   records.push_back(temp_char);

   delete[] temp_char;

}

`while (!o.eof())`, is wrong, and `delete[] temp_char;` (unless `records.push_back(temp_char)` copies the pointee). — Daniel Fischer, Jan 12 '13 at 21:48
@antimatter is this ultimately to read strings from an input stream, separated by whitespace, and store them into the `records` collection? — WhozCraig, Jan 12 '13 at 22:00
@antimatter : What type is `records`? Why would you think this question is answerable without that information? — ildjarn, Jan 12 '13 at 22:02
Vague title is unlikely to be useful to future visitors to the site. — Raymond Chen, Jan 12 '13 at 22:49

score 2 · Answer 1 · answered Jan 12 '13 at 21:52

2

o.eof() only becomes true after you tried to read past the end of the stream, so you shouldn't use while(!o.eof()) for the loop control.

Unless recods.push_back(temp_char) copies the pointed-to array, records will contain a dangling pointer after delete[] temp_char;

while(true) {
    temp_char = new char[31];
    o >> temp_char;
    if (!o) {
        delete[] temp_char;
        break;
    }
    records.push_back(temp_char);
}

looks better (although I'm sure it's not idiomatic).

Of course, using std::string would relieve you of the memory management.

answered Jan 12 '13 at 21:52

Daniel Fischer

174,737
16
293
422

+1, or better still, seriously consider why you're *not* using `std::string` both here and in your `records` collection. – WhozCraig Jan 12 '13 at 21:54
@Daniel Fischer records is vector so it copies temp_char to string, right? and then deletes temp_char... – Jan 12 '13 at 21:55
@antimatter Okay, in that case, the "unless" would apply. The `o.eof()` part stays, however. – Daniel Fischer Jan 12 '13 at 21:58
!o.eof() is okay, because while loop must iterate through all stream in object o, until the end. So, there is nothing wrong with this code? – Jan 12 '13 at 22:02
@antimatter No, at the end of the stream, you would attempt another extraction which would fail, since there is no more input, and you'd `push_back` a `char*` with indeterminate contents. If there is no 0-byte in the allocated block, the string constructor would read out of bounds. – Daniel Fischer Jan 12 '13 at 22:05
@DanielFischer That will not happen, because when stream comes to the end, while loop will break. – Jan 12 '13 at 22:18
@antimatter So it's a custom self-implemented stream class, and not a `std::ifstream` or such? – Daniel Fischer Jan 12 '13 at 22:20
@antimatter Look at [this](http://stackoverflow.com/questions/5605125/why-is-iostreameof-inside-a-loop-condition-considered-wrong) for `eof()`. – Daniel Fischer Jan 12 '13 at 22:23
@DanielFischer that's stream class from OTL_stream object, for connecting to the database (see: otl.sourceforge.net). I wrote a few similar functions like this, before, and never had problems with otl_stream. Saw !o.eof() in Examples section on official webpage. – Jan 12 '13 at 23:08
@antimatter The docs say: "This function has the same meaning as the eof() function in C++ streams". If that's wrong, your use of `!o.eof()` could be correct. – Daniel Fischer Jan 12 '13 at 23:20

score 0 · Answer 2 · answered Jan 12 '13 at 22:34

I wouldn't reallocate the string each time I read a word. And if the last iteration reads spaces, an empty string will be pushed back.

    char* temp_char = new char[BUF_SZ];

    while ( ! o.eof() ) {

       o>>temp_char;
if( *temp_char != '\0' ) // If not an empty string
       records.push_back(temp_char);

    }

    delete[] temp_char;

Cannot find the error in C++ code

2 Answers2