ifstream problem?

[kazik.lakomy]

Hi,
could you help me with this stuff, it compiles fine, but when starting
the program there is segmentation fault. I enclose a message from
debugger:
----------
Program received signal SIGSEGV, Segmentation fault.
0x0000000000400ce0 in ReadInitialConditions (TabX=0x7fffb62f1880,
TabY=0x7fffb62f1480, TableSize=128, InitCondFile=@0x7fffb62f1c80)
at readprog.cpp:9
9 while(! InitCondFile.eof() ){
----------

This is the code:

#include <iostream>
#include <fstream>
using namespace std;

void ReadInitialConditions(double* TabX, double* TabY, int TableSize,
ifstream & InitCondFile){
int XorYCounter = 0;
int XYPairsCounter = 0;
if(InitCondFile.is_open()){
while(! InitCondFile.eof() ){
double num;
InitCondFile>>num;
if((XorYCounter % 2) == 0)
TabX[XYPairsCounter] = num;
if((XorYCounter % 2) != 0){
TabY[XYPairsCounter] = num;
XYPairsCounter++;
}
XorYCounter++;
}
InitCondFile.close();
}
else
cout<<"Unable to open file";
}

int main(){
ifstream InitCondFile("groundsol.dat");
const int TableSize = 128;
double TabX[TableSize];
double TabY[TableSize];
ReadInitialConditions(TabX, TabY, TableSize, InitCondFile);
for(int j=0; j<TableSize; j++)
cout<<"x"<<j<<": "<<TabX[j]<<" y"<<j<<": "<<TabY[j]<<endl;
return 0;
}

 

[kazik.lakomy]

Yes, the file for sure exist. It contains two columns of numbers
generated by [the form that is suitable for Gnuplot].

"Interesting" is that when I put it like that:
int main(){
const int TabSize = 128;
double TabX[TabSize];
double TabY[TabSize];
int XorYCounter = 0;
int XYPairsCounter = 0;
ifstream myfile ("groundsol.dat");
if (myfile.is_open())
{
while (! myfile.eof() )
{
double num;
myfile>>num;
if((XorYCounter % 2) == 0)
TabX[XYPairsCounter] = num;
if((XorYCounter % 2) != 0){
TabY[XYPairsCounter] = num;
XYPairsCounter++;
}
XorYCounter++;
}
myfile.close();
}
else cout << "Unable to open file";
for(int j=0; j<TabSize; j++)
cout<<"x"<<j<<": "<<TabX[j]<<" y"<<j<<": "<<TabY[j]<<endl;
return 0;
}

so that everything is exactly the same except all is happening in main
(), then it works perfect. However, needles to say, it's not what I
would like to have. I would like to have a function reading data from
some external file.

 

[kazik.lakomy]

Sorry for spelling mistakes.

 

[kazik.lakomy]

Thanks for spelling support
Yes, the data file contains two columns of 128 rows of numbers, now
I'm trying to understand your conclusion about the memory. Hope it
will work out.

 

[kazik.lakomy]

Ok, I'll be honest. I didn't understand your suggestion about not
enough memory. However I had an intuition that it might be something
with giving the function stream in argument. So I've changes this in
that way:

void ReadInitialConditions(double* TabX, double* TabY, int TableSize){
ifstream InitCondFile("groundsol.dat");
int XorYCounter = 0;
int XYPairsCounter = 0;
if(InitCondFile.is_open()){
while(! InitCondFile.eof() ){
double num;
InitCondFile>>num;
if((XorYCounter % 2) == 0)
TabX[XYPairsCounter] = num;
if((XorYCounter % 2) != 0){
TabY[XYPairsCounter] = num;
XYPairsCounter++;
}
XorYCounter++;
}
InitCondFile.close();
}
else
cout<<"Unable to open file";
}
so, basically, I've resigned from giving the stream as an argument and
I've put it inside the function [that's the only one change], and now
it's working perfectly. So OK, it works, but still I don't know what
was wrong with giving the stream as an argument. Any ideas?

 

[kazik.lakomy]

Hi,
yes, I'm totally sure about it. To give you an argument please see
this example from http://www.cplusplus.com/doc/tutorial/files.html:
----------
// reading a text file
#include <iostream>
#include <fstream>
#include <string>
using namespace std;

int main () {
string line;
ifstream myfile ("example.txt");
if (myfile.is_open())
{
while (! myfile.eof() )
{
getline (myfile,line);
cout << line << endl;
}
myfile.close();
}

else cout << "Unable to open file";

return 0;
}
----------
Here as you can see there's also no "a counter" that would tell the
computer to read next and next line. It's somehow like getline would
know that "ok, I have read the first line, now I will read the second
line". What I mean is there is no int number changing its value from
one to two after reading the first line just to inform the program
that in a moment it will be reading line no. 2. I have to admit that I
don't understand it very well [though I'd love to] but that's just the
way it is and works. If you had any idea why it's like that don't
hesitate to write.

 

[Alf P. Steinbach]

* (e-mail address removed):

Hi,
yes, I'm totally sure about it. To give you an argument please see
this example from http://www.cplusplus.com/doc/tutorial/files.html:
----------
// reading a text file
#include <iostream>
#include <fstream>
#include <string>
using namespace std;

int main () {
string line;
ifstream myfile ("example.txt");
if (myfile.is_open())
{
while (! myfile.eof() )
{
getline (myfile,line);
cout << line << endl;
}
myfile.close();
}

else cout << "Unable to open file";

return 0;
}
----------
Here as you can see there's also no "a counter" that would tell the
computer to read next and next line. It's somehow like getline would
know that "ok, I have read the first line, now I will read the second
line". What I mean is there is no int number changing its value from
one to two after reading the first line just to inform the program
that in a moment it will be reading line no. 2. I have to admit that I
don't understand it very well [though I'd love to] but that's just the
way it is and works. If you had any idea why it's like that don't
hesitate to write.

Above is good example of how to not do things.

Is it described as such on the tutorial page?

Idiomatic loop would be

while( getline( myfile, line ) )
{
cout << line << endl;
}

which doesn't try to output the last line twice.


Cheers & hth.,

- Alf

 

[kazik.lakomy]

Yes, this is exactly what is in tutorial page whose address I've
enclosed.
And any ideas what was wrong in giving a stream as an argument to a
function [I mean previous, not working version]?

 

[Alf P. Steinbach]

* (e-mail address removed):

Yes, this is exactly what is in tutorial page whose address I've
enclosed.

Oh dear.

And any ideas what was wrong in giving a stream as an argument to a
function [I mean previous, not working version]?

No problem with *that*.

And it seems to me that Victor did a good job of dissecting that code.

However, my take...

#include <iostream>
#include <fstream>
using namespace std;

void ReadInitialConditions(double* TabX, double* TabY, int TableSize,
ifstream & InitCondFile){

Too many arguments, plus arguments of raw types; use std::vector.

int XorYCounter = 0;
int XYPairsCounter = 0;
if(InitCondFile.is_open()){
while(! InitCondFile.eof() ){
double num;
InitCondFile>>num;

Forgetting to check for failure.

if((XorYCounter % 2) == 0)
TabX[XYPairsCounter] = num;

Forgetting that raw array may not have enough room (buffer overflow).

if((XorYCounter % 2) != 0){

Repeating condition instead of using 'else'.

TabY[XYPairsCounter] = num;
XYPairsCounter++;

Non-idiomatic. Either hoist that postfix '++' into the indexing, or use prefix
'++'. I.e. ask only for what you actually want done, even here where it doesn't
matter regarding generated code.

}
XorYCounter++;
}
InitCondFile.close();

Confused resource ownership. The code that opened the file should close it. Ideally.

}
else
cout<<"Unable to open file";

In a non-interactive program, which this is, send error messages to the standard
error steam, not the standard output stream.

Also, confusion of responsibility, whose job it is to report errors to the user.

For example, this routine couldn't be used in a GUI program, because of this.

}

int main(){
ifstream InitCondFile("groundsol.dat");
const int TableSize = 128;
double TabX[TableSize];
double TabY[TableSize];
ReadInitialConditions(TabX, TabY, TableSize, InitCondFile);
for(int j=0; j<TableSize; j++)
cout<<"x"<<j<<": "<<TabX[j]<<" y"<<j<<": "<<TabY[j]<<endl;

Preferentially use braces around loop bodies, otherwise they'll bite you.

return 0;

This explicitly says the program succeeded, when it may very well have failed.

}

</dissection>


The following alternative code is just typed in, never handled by compiler:

<alternative>
#include <iostream>
#include <fstream>
#include <vector>
#include <stdlib.h>

using namespace std;

struct Condition
{
int x;
int y;
};

istream& operator>>( istream& stream, Condition& cond )
{
return (stream >> cond.x >> cond.y);
}

typedef vector<Condition> ConditionVec;

bool readFrom( istream& stream, ConditionVec& conditions )
{
ConditionVec result;
Condition cond;

while( stream >> cond ) { result.push_back( cond ); }

bool const ok = stream.eof();
if( ok ) { result.swap( conditions ); }
return ok;
}

int main()
{
ifstream condFile( "groundsol.dat" );
if( !condFile )
{
cerr << "Unable to open file." << endl;
return EXIT_FAILURE;
}

ConditionVec conditions;
if( !readFrom( condFile, conditions ) )
{
cerr << "File contained invalid data." << endl;
return EXIT_FAILURE;
}

for( size_t i = 0; i < conditions.size(); ++i )
{
Condition const& c = conditions.at( i );
cout << "x" << j << ": " << c.x << " y" << i << ": " << x.y << endl;
}
return EXIT_SUCCESS;
}
</alternative>

Of course this code will probably not even compile. But it should give you some
ideas about how to structure things. I've abstained from using exceptions in
order to keep the code simple for someone who possibly isn't used to them.


Cheers & hth.,

- Alf

 

[kazik.lakomy]

Wow,
thank you, it's a great "review-comment".
It is very helpful. Respect for that

 

[James Kanze]

(e-mail address removed) wrote:

You should probably change this to

while( InitCondFile.good() ) {

In no case. Both are questionable. The idiomatic way of doing
this is:

double num ;
while ( InitCondFile >> num ) {
...
}

Also make this a condition:

if (InitCondFile >> num) {

So why not move the condition up to the top of the loop, and be
done with it?

 

[James Kanze]

* (e-mail address removed):

[...]

Forgetting to check for failure.

Even if he checks for failure here, he will end up in an endless
loop if the failure isn't due to end of file. It's essential
that the loop terminate if the input fails, which is why the
standard idiom puts the test in the loop control:

while ( InitCondFile >> num ) ...

[...]

TabY[XYPairsCounter] = num;
XYPairsCounter++;

Non-idiomatic.

Except in about 99% of the working code I see. There's
absolutely nothing "unidiomatic" or criticizable with the above
(except that it should be TabY.push_back( num ), as you
mentionned in the part I cut).

Either hoist that postfix '++' into the indexing, or use
prefix '++'. I.e. ask only for what you actually want done,
even here where it doesn't matter regarding generated code.

Both ++ operators return a value that he doesn't need, so in a
certain sense, both are doing more than he requires. If you
don't need the return value, then generally speaking, there's no
real reason (other than a lot of handwaving about what a
compiler might do---not backed up by actual measurements) to
prefer one over the other.

 

[Alf P. Steinbach]

* James Kanze:

Both ++ operators return a value that he doesn't need, so in a
certain sense, both are doing more than he requires. If you
don't need the return value, then generally speaking, there's no
real reason (other than a lot of handwaving about what a
compiler might do---not backed up by actual measurements) to
prefer one over the other.

If you're trying to impress the world with your ability to restate my comments
in new terms (here restating "doesn't matter regarding generated code" in terms
of less certain measurements) that, in the context you do that, imply something
else than the original statement, well, I don't think it's a great idea.

I find it curious that even people with as much experience as you have think
that coding practices should be determined by the micro-efficiency of generated
machine code in the most common cases.

It seems that that argument is so deeply entrenched in your mind that in spite
of quoting text that clearly spells out, in teaspoon mode, that efficiency
doesn't matter in this case, you think that the argument is made that it does.


Cheers & hth.,

- Alf

 

[jerry.coffin]

On Feb 26, 3:22 pm, (e-mail address removed) wrote:

[ ... ]

void ReadInitialConditions(double* TabX, double* TabY, int TableSize,ifstream& InitCondFile){
 int XorYCounter = 0;
 int XYPairsCounter = 0;
 if(InitCondFile.is_open()){
   while(! InitCondFile.eof() ){
     double num;
     InitCondFile>>num;
     if((XorYCounter % 2) == 0)
       TabX[XYPairsCounter] = num;
     if((XorYCounter % 2) != 0){
       TabY[XYPairsCounter] = num;
       XYPairsCounter++;
     }
     XorYCounter++;
   }
   InitCondFile.close();
 }
 else
   cout<<"Unable to open file";

}

You've gotten a number of replies, and while all of them have
addressed the immediate problem you ran into, I don't think any of
them has really done a very good job of looking at the root problem
you're solving, and showing how to solve that basic problem as well as
possible.

From the looks of your code, you apparently have a file of X and Y
coordinates, that have to be read into arrays. First of all, unless
it's really crucial that the X's and Y's be in separate arrays, I'd
start by creating a type for the X,Y coordinate pair:

struct coordinate {
double x, y;
};

Then I'd define an operator to read in a coordinate pair:

istream &operator>>(istream &infile, coordinate &c) {
infile >> c.x;
infile >> c.y;
return infile;
}

To support printing out the coordinates, I'd add an inserter as well:

ostream &operator<<(ostream &outfile, coordinate const &c) {
return outfile << c.x << ": " << c.y;
}

Then I'd have main create a vector of those, and copy the data into
the vector:

int main() {
vector<coordinate> table;

ifstream initCondFile("groundso1.dat");

copy(istream_iterator<coordinate>(initCondFile),
istream_iterator<coordinate>(),
back_inserter(table));

copy(table.begin(),
table.end(),
ostream_iterator<coordinate>(cout, "\n"));
};

This completely eliminates having to write the loops for reading or
writing the coordinates, allocating the right amount of space to hold
the coordinates, getting the conditions for dealing with the end of
file correct, etc.