Finding end of file while reading from it

Question

void graph::fillTable()
{
  ifstream fin;
  char X;
  int slot=0;

  fin.open("data.txt");

  while(fin.good()){

  fin>>Gtable[slot].Name;
  fin>>Gtable[slot].Out;
  cout<<Gtable[slot].Name<<endl;
  for(int i=0; i<=Gtable[slot].Out-1;i++)
    {
      **//cant get here**
    fin>>X;
    cout<<X<<endl;
    Gtable[slot].AdjacentOnes.addFront(X);
    }
  slot++;
  }
 fin.close();
}

That's my code, basically it does exactly what I want it to but it keeps reading when the file is not good anymore. It'll input and output all the things I'm looking for, and then when the file is at an end, fin.good() apparently isn't returning false. Here is the text file.

A 2 B F

B 2 C G

C 1 H

H 2 G I

I 3 A G E

F 2 I E

and here is the output

A
B
F
B
C
G
C
H
H
G
I
I
A
G
E
F
I
E

Segmentation fault

-

Here's is Gtable's type.

struct Gvertex:public slist
  {
    char Name;
    int VisitNum;
    int Out;
    slist AdjacentOnes;
    //linked list from slist
  };

I'm expecting it to stop after outputting 'E' which is the last char in the file. The program never gets into the for loop again after reading the last char. I can't figure out why the while isn't breaking.

Answer 1

Your condition in the while loop is wrong. ios::eof() isn't predictive; it will only be set once the stream has attempted (internally) to read beyond end of file. You have to check after each
input.

The classical way of handling your case would be to define a >> function for GTable, along the lines of:

std::istream&
operator>>( std::istream& source, GTable& dest )
{
    std::string line;
    while ( std::getline( source, line ) && line.empty() ) {
    }
    if ( source ) {
        std::istringstream tmp( line );
        std::string name;
        int count;
        if ( !(tmp >> name >> count) ) {
            source.setstate( std::ios::failbit );
        } else {
            std::vector< char > adjactentOnes;
            char ch;
            while ( tmp >> ch ) {
                adjactentOnes.push_back( ch );
            }
            if ( !tmp.eof() || adjactentOnes.size() != count ) {
                source.setstate( std::ios::failbit );
            } else {
                dest.Name = name;
                dest.Out = count;
                for ( int i = 0; i < count; ++ i ) {
                    dest.AdjacentOnes.addFront( adjactentOnes[ i ] );
                }
            }
        }
    }
    return source;
}

(This was written rather hastily. In real code, I'd almost certainly factor the inner loop out into a separate function.)

Note that:

• We read line by line, in order to verify the format (and to allow resynchronization in case of error).

• We set failbit in the source stream in case of an input error.

• We skip empty lines (since your input apparently contains them).

• We do not modify the target element until we are sure that the input is correct.

One we have this, it is easy to loop over all of the elements:

int slot = 0;
while ( slot < GTable.size() && fin >> GTable[ slot ] ) {
    ++ slot;
}
if ( slot != GTable.size )
    //  ... error ...

EDIT:

I'll point this out explicitly, because the other people responding seem to have missed it: it is absolutely imperative to ensure that you have the place to read into before attempting the read.

EDIT 2:

Given the number of wrong answers this question is receiving, I would like to stress:

• Any use of fin.eof() before the input is known to fail is wrong.

• Any use of fin.good(), period, is wrong.

• Any use of one of the values read before having tested that the input has succeeded is wrong. (This doesn't prevent things like fin >> a >> b, as long as neither a or b are used before the success is tested.)

• Any attempt to read into Gtable[slot] without ensuring that slot is in bounds is wrong.

With regards to eof() and good():

The base class of istream and ostream defines three “error” bits: failbit, badbit and eofbit. It's important to understand when these are set: badbit is set in case of a non-recoverable hardward error (practically never, in fact, since most implementations can't or don't detect such errors); and failbit is set in any other case the input fails—either no data available (end of file), or a format error ("abc" when inputting an int, etc.). eofbit is set anytime the streambuf returns EOF, whether this causes the input to fail or not! Thus, if you read an int, and the stream contains "123", without trailing white space or newline, eofbit will be set (since the stream must read ahead to know where the int ends); if the stream contains "123\n", eofbit will not be set. In both cases, however, the input succeeds, and failbit will not be set.

To read these bits, there are the following functions (as code, since I don't know how to get a table otherwise):

eof():   returns eofbit
bad():   returns badbit
fail():  returns failbit || badbit
good():  returns !failbit && !badbit && !eofbit

operator!():      returns fail()
operator void*(): returns fail() ? NULL : this
    (typically---all that's guaranteed is that !fail() returns non-null.)

Given this: the first check must always be fail() or one of the operator (which are based on fail). Once fail() returns true, we can use the other functions to determine why:

if ( fin.bad() ) {
    //  Serious problem, disk read error or such.
} else if ( fin.eof() ) {
    //  End of file: there was no data there to read.
} else {
    //  Formatting error: something like "abc" for an int
}

Practically speaking, any other use is an error (and any use of good() is an error—don't ask me why the function is there).

Answer 2

Slightly slower but cleaner approach:

void graph::fillTable()
{
  ifstream fin("data.txt");
  char X;
  int slot=0;

  std::string line;

  while(std::getline(fin, line))
  {
    if (line.empty()) // skip empty lines
      continue;

    std::istringstream sin(line);
    if (sin >> Gtable[slot].Name >> Gtable[slot].Out && Gtable[slot].Out > 0)
    {
      std::cout << Gtable[slot].Name << std::endl;
      for(int i = 0; i < Gtable[slot].Out; ++i)
      {
        if (sin >> X)
        {
          std::cout << X << std::endl;
          Gtable[slot].AdjacentOnes.addFront(X);
        }
      }
      slot++;
    }
  }
}

If you still have issues, it's not with file reading...

Answer 3

The file won't fail until you actually read from past the end of file. This won't occur until the fin>>Gtable[slot].Name; line. Since your check is before this, good can still return true.

One solution would be to add additional checks for failure and break out of the loop if so.

fin>>Gtable[slot].Name;
fin>>Gtable[slot].Out;
if(!fin) break;

This still does not handle formatting errors in the input file very nicely; for that you should be reading line by line as mentioned in some of the other answers.

Answer 4

Try moving first two reads in the while condition:

// assuming Gtable has at least size of 1

while( fin>>Gtable[slot].Name && fin>>Gtable[slot].Out ) {
    cout<<Gtable[slot].Name<<endl;
    for(int i=0; i<=Gtable[slot].Out-1;i++) {
        fin>>X;
        cout<<X<<endl;
        Gtable[slot].AdjacentOnes.addFront(X);
    }
  slot++;

  //EDIT:

  if (slot == table_size) break;
}

Edit: As per James Kanze's comment, you're taking an adress past the end of Gtable array, which is what causes segfault. You could pass the size of Gtable as argument to your fillTable() function (f.ex. void fillTable(int table_size)) and check slot is in bounds before each read.

Answer 5

*Edited in response to James' comment - the code now uses a good() check instead of a !eof() check, which will allow it to catch most errors. I also threw in an is_open() check to ensure the stream is associated with the file.*

Generally, you should try to structure your file reading in a loop as follows:

ifstream fin("file.txt");
char a = '\0';
int b = 0;
char c = '\0';

if (!fin.is_open())
    return 1; // Failed to open file.

// Do an initial read. You have to attempt at least one read before you can
// reliably check for EOF.
fin >> a;

// Read until EOF
while (fin.good())
{
    // Read the integer
    fin >> b;

    // Read the remaining characters (I'm just storing them in c in this example)
    for (int i = 0; i < b; i++)
        fin >> c;

    // Begin to read the next line. Note that this will be the point at which
    // fin will reach EOF. Since it is the last statement in the loop, the
    // file stream check is done straight after and the loop is exited.
    // Also note that if the file is empty, the loop will never be entered.
    fin >> a;
}

fin.close();

This solution is desirable (in my opinion) because it does not rely on adding random breaks inside the loop, and the loop condition is a simple good() check. This makes the code easier to understand.