STL: Could you make this snippet more efficient

Discussion in 'C++' started by pedagani@gmail.com, Dec 3, 2007.

  1. Guest

    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.
     
    , Dec 3, 2007
    #1
    1. Advertising

  2. Re: Could you make this snippet more efficient

    wrote:
    > 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
    --
    Please remove capital 'A's when replying by e-mail
    I do not respond to top-posted replies, please don't ask
     
    Victor Bazarov, Dec 3, 2007
    #2
    1. Advertising

  3. Guest

    Re: Could you make this snippet more efficient

    On Dec 3, 11:37 am, "Victor Bazarov" <> wrote:
    > wrote:
    > > 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
    > --
    > Please remove capital 'A's when replying by e-mail
    > I do not respond to top-posted replies, please don't ask


    *Correction* 'pckset' should have been 'modpckset'.
     
    , Dec 3, 2007
    #3
  4. Re: Could you make this snippet more efficient

    wrote:
    > On Dec 3, 11:37 am, "Victor Bazarov" <> wrote:
    >> wrote:
    >>> 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
    >> --
    >> Please remove capital 'A's when replying by e-mail
    >> I do not respond to top-posted replies, please don't ask

    >
    > *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
    --
    Please remove capital 'A's when replying by e-mail
    I do not respond to top-posted replies, please don't ask
     
    Victor Bazarov, Dec 3, 2007
    #4
  5. red floyd Guest

    wrote:
    > 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?

    > {
    > 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.
     
    red floyd, Dec 3, 2007
    #5
  6. Guest

    Re: Could you make this snippet more efficient

    On Dec 3, 11:50 am, "Victor Bazarov" <> wrote:
    > wrote:
    > > On Dec 3, 11:37 am, "Victor Bazarov" <> wrote:
    > >> wrote:
    > >>> 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
    > >> --
    > >> Please remove capital 'A's when replying by e-mail
    > >> I do not respond to top-posted replies, please don't ask

    >
    > > *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
    > --
    > Please remove capital 'A's when replying by e-mail
    > I do not respond to top-posted replies, please don't ask


    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.
     
    , Dec 3, 2007
    #6
  7. Guest

    On Dec 3, 11:51 am, red floyd <> wrote:
    > wrote:
    > > 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?
    >
    > > {
    > > 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.


    Thank you, got it.
     
    , Dec 3, 2007
    #7
  8. Re: Could you make this snippet more efficient

    wrote:
    > On Dec 3, 11:50 am, "Victor Bazarov" <> wrote:
    >> wrote:
    >>> On Dec 3, 11:37 am, "Victor Bazarov" <>
    >>> wrote:
    >>>> wrote:
    >>>>> 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
    >>>> --
    >>>> Please remove capital 'A's when replying by e-mail
    >>>> I do not respond to top-posted replies, please don't ask

    >>
    >>> *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
    >> --
    >> Please remove capital 'A's when replying by e-mail
    >> I do not respond to top-posted replies, please don't ask

    >
    > 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::eek: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
    --
    Please remove capital 'A's when replying by e-mail
    I do not respond to top-posted replies, please don't ask
     
    Victor Bazarov, Dec 3, 2007
    #8
  9. Mark P Guest

    wrote:
    > 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
     
    Mark P, Dec 3, 2007
    #9
  10. Daniel T. Guest

    wrote:

    > 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() ) );
    }
     
    Daniel T., Dec 3, 2007
    #10
  11. Guest

    On Dec 3, 3:23 pm, "Daniel T." <> wrote:
    > wrote:
    > > 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() ) );
    > }


    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() ) );
    //...
    }
     
    , Dec 3, 2007
    #11
  12. Daniel T. wrote:
    > [..]
    > 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
    --
    Please remove capital 'A's when replying by e-mail
    I do not respond to top-posted replies, please don't ask
     
    Victor Bazarov, Dec 3, 2007
    #12
  13. Daniel T. Guest

    "Victor Bazarov" <> wrote:
    > Daniel T. wrote:
    > > [..]
    > > 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.
     
    Daniel T., Dec 4, 2007
    #13
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Smithers
    Replies:
    7
    Views:
    636
    Smithers
    May 18, 2005
  2. CodeCracker
    Replies:
    25
    Views:
    3,827
    Richard Herring
    May 23, 2005
  3. KK
    Replies:
    14
    Views:
    625
    Bo Persson
    Jun 30, 2006
  4. Replies:
    2
    Views:
    497
    Dan Andrews
    Jun 16, 2007
  5. Diwa
    Replies:
    0
    Views:
    1,039
Loading...

Share This Page