STL: Could you make this snippet more efficient

[pedagani]

Dear comp.lang.c++,

Could you make this snippet more efficient? As you see I have too many
variables introduced in the code.

//Read set of integers from a file on line by line basis in a STL set
//fp is pre-defined
for(;!fp.eof()
{
string linestr;
getline(fp,linestr);
istringstream strstreamline(linestr);
istream_iterator<unsigned int> intstream(strstreamline);
set<unsigned int> pckset;
copy ( intstream , istream_iterator<unsigned int>() ,
inserter(modpckset,modpckset.begin ( ) ));
}

Thanks in advance.

 

[Victor Bazarov]

Dear comp.lang.c++,

Could you make this snippet more efficient? As you see I have too many
variables introduced in the code.

//Read set of integers from a file on line by line basis in a STL set
//fp is pre-defined
for(;!fp.eof()
{
string linestr;
getline(fp,linestr);
istringstream strstreamline(linestr);
istream_iterator<unsigned int> intstream(strstreamline);
set<unsigned int> pckset;

What's that for?

copy ( intstream , istream_iterator<unsigned int>() ,
inserter(modpckset,modpckset.begin ( ) ));
}

V

 

[pedagani]

What's that for?


V

*Correction* 'pckset' should have been 'modpckset'.

 

[Victor Bazarov]

*Correction* 'pckset' should have been 'modpckset'.

OK. The entire snippet can be made more efficient by removing
the lines between

getline(fp, linestr);

and

}

; they do essentially no work that would have side effects.
The creation of a stringstream, reading stuff from it, and
putting those unsigned ints into a local set are a waste of
CPU cycles.

V

 

[red floyd]

Dear comp.lang.c++,

Could you make this snippet more efficient? As you see I have too many
variables introduced in the code.

//Read set of integers from a file on line by line basis in a STL set
//fp is pre-defined
for(;!fp.eof()

Hint. The for statement does not do what you think it does.
Also, why are you using a for() instead of a while() in this situation?

 

[pedagani]

OK. The entire snippet can be made more efficient by removing
the lines between

getline(fp, linestr);

and

}

; they do essentially no work that would have side effects.
The creation of a stringstream, reading stuff from it, and
putting those unsigned ints into a local set are a waste of
CPU cycles.

V

Insightful perspective. It just me that I dont have to necessarily
deal them as integers and could consider them as strings instead.
Appreciate your help.

However, I need to store the parsed result in a container on which I
could apply set_difference algorithm. Looks like I must resort to
stringstream in this case. Please correct me if I am wrong.

Best.

 

[pedagani]

Hint. The for statement does not do what you think it does.
Also, why are you using a for() instead of a while() in this situation?

Thank you, got it.

 

[Victor Bazarov]

Insightful perspective. It just me that I dont have to necessarily
deal them as integers and could consider them as strings instead.
Appreciate your help.

However, I need to store the parsed result in a container on which I
could apply set_difference algorithm. Looks like I must resort to
stringstream in this case. Please correct me if I am wrong.

If you have to convert them, you should use some kind of conversion
function, be it strtod, sscanf, or istream:perator>>. Actually,
it is quite possible that operator>> internally uses strtod.

If storing them in a set is a requirements, you probably need to
do your 'copy' with the second argument being 'inserter' for some
other, non-local set.

It is possible that once the string is read, you might have better
luck using 'strtod' yourself (essentially replacing what operator>>
does, skipping WS, etc.) However, without measuring it's hard to
say what area of your algorithm you need to address.

V

 

[Mark P]

Dear comp.lang.c++,

Could you make this snippet more efficient? As you see I have too many
variables introduced in the code.

//Read set of integers from a file on line by line basis in a STL set
//fp is pre-defined
for(;!fp.eof()
{
string linestr;
getline(fp,linestr);
istringstream strstreamline(linestr);
istream_iterator<unsigned int> intstream(strstreamline);
set<unsigned int> pckset;
copy ( intstream , istream_iterator<unsigned int>() ,
inserter(modpckset,modpckset.begin ( ) ));
}

Efficiency doesn't have much to do with number of variables, however, it
probably *is* more efficient to stream your results into a vector, sort
it, and then apply set_difference (which, of course, doesn't actually
require a std::set). One sort at the end is likely to be faster than
maintaining a sorted structure throughout the construction.

Mark

 

[Daniel T.]

Could you make this snippet more efficient? As you see I have too many
variables introduced in the code.

//Read set of integers from a file on line by line basis in a STL set
//fp is pre-defined
for(;!fp.eof()
{
string linestr;
getline(fp,linestr);
istringstream strstreamline(linestr);
istream_iterator<unsigned int> intstream(strstreamline);
set<unsigned int> pckset;
copy ( intstream , istream_iterator<unsigned int>() ,
inserter(modpckset,modpckset.begin ( ) ));
}

Thanks in advance.

First let's clean up the code you have.

set< unsigned int > modpckset; // has to be defined outside the loop
string line;
while ( getline( fp, line ) ) // proper way to read until end of file
{
istringstream ss( line );
copy( istream_iterator< unsigned int >( ss ),
istream_iterator< unsigned int >(),
inserter( modpckset, modpckset.begin() ) );
}

With the above code, getline will read in a line of the file at a time,
then parse it out to a bunch of unsigned ints. We don't need to do the
extra step:

set< unsigned int > modpckset;
copy( istream_iterator< unsigned int >( fp ),
istream_iterator< unsigned int >(),
inserter( modpckset, modpckset.begin() ) );

The above will do the same thing.

Now, if your goal was to read each line into a separate set, say you
want to end up with a vector of sets, then you would need to read each
line separately.

vector< set< unsigned int > > all_sets;
string line;
while ( getline( fp, line ) )
{
istringstream ss( line );
all_sets.push_back( set< unsigned int >() );
copy( istream_iterator< unsigned int >( ss ),
istream_iterator< unsigned int >(),
inserter( all_sets.back(), all_sets.back().begin() ) );
}

 

[pedagani]

First let's clean up the code you have.

set< unsigned int > modpckset; // has to be defined outside the loop
string line;
while ( getline( fp, line ) ) // proper way to read until end of file
{
istringstream ss( line );
copy( istream_iterator< unsigned int >( ss ),
istream_iterator< unsigned int >(),
inserter( modpckset, modpckset.begin() ) );

}

With the above code, getline will read in a line of the file at a time,
then parse it out to a bunch of unsigned ints. We don't need to do the
extra step:

set< unsigned int > modpckset;
copy( istream_iterator< unsigned int >( fp ),
istream_iterator< unsigned int >(),
inserter( modpckset, modpckset.begin() ) );

The above will do the same thing.

Now, if your goal was to read each line into a separate set, say you
want to end up with a vector of sets, then you would need to read each
line separately.

vector< set< unsigned int > > all_sets;
string line;
while ( getline( fp, line ) )
{
istringstream ss( line );
all_sets.push_back( set< unsigned int >() );
copy( istream_iterator< unsigned int >( ss ),
istream_iterator< unsigned int >(),
inserter( all_sets.back(), all_sets.back().begin() ) );
}

Very nice. If its not too much clutter one could do the same as

set< unsigned int > modpckset; // has to be defined outside the loop
string line;
while ( getline( fp, line ) ) // proper way to read until end of file
{
copy( istream_iterator< unsigned int >( istringstream (line) ),
istream_iterator< unsigned int >(),
inserter( modpckset, modpckset.begin() ) );
//...
}

 

[Victor Bazarov]

[..]
set< unsigned int > modpckset; // has to be defined outside the loop
string line;
while ( getline( fp, line ) ) // proper way to read until end of file
{
istringstream ss( line );
copy( istream_iterator< unsigned int >( ss ),
istream_iterator< unsigned int >(),
inserter( modpckset, modpckset.begin() ) );
}

With the above code, getline will read in a line of the file at a
time, then parse it out to a bunch of unsigned ints. We don't need to
do the extra step:

set< unsigned int > modpckset;
copy( istream_iterator< unsigned int >( fp ),
istream_iterator< unsigned int >(),
inserter( modpckset, modpckset.begin() ) );

The above will do the same thing.

Really? Will it? What if it can't copy (there is 'O' instead of '0'
somewhere in the file)? Will it attempt to read again from the next
line?

[..]

V

 

[Daniel T.]

[..]
set< unsigned int > modpckset; // has to be defined outside the loop
string line;
while ( getline( fp, line ) ) // proper way to read until end of file
{
istringstream ss( line );
copy( istream_iterator< unsigned int >( ss ),
istream_iterator< unsigned int >(),
inserter( modpckset, modpckset.begin() ) );
}

With the above code, getline will read in a line of the file at a
time, then parse it out to a bunch of unsigned ints. We don't need to
do the extra step:

set< unsigned int > modpckset;
copy( istream_iterator< unsigned int >( fp ),
istream_iterator< unsigned int >(),
inserter( modpckset, modpckset.begin() ) );

The above will do the same thing.

Really? Will it? What if it can't copy (there is 'O' instead of '0'
somewhere in the file)? Will it attempt to read again from the next
line?

Sorry, at some point while writing the post, I had written that I was
assuming that the file contained all unsigned ints. I must have
accidentally cut that line out.