7

Presently, I set the value of a std::vector<char> from an std::ostringstream as follows:

void
foo(std::vector<char> &data, std::stringstream &stream) {
  data = std::vector<char>(stream.str().begin(), stream.str().end());
}

I'm wondering if there is a more efficient way to do this with STL in C++ or whether the method I give here is considered appropriate? Would I be better off using std::stringstream instead?

WilliamKF
  • 41,123
  • 68
  • 193
  • 295
  • 8
    I'm not sure how efficient that is, but it *is* incorrect. The two calls to `.str()` return different objects. – Robᵩ May 30 '12 at 19:53
  • Thanks for pointing out that error, thought I was getting a reference not a copy from str(). – WilliamKF May 30 '12 at 20:22

4 Answers4

12

As pointed out in comments, your code is incorrect due to the two calls to str(). In order to improve efficiency you can avoid creating a temporary vector, like this:

void foo(std::vector<char> &data, std::stringstream &stream) {
    const std::string& str = stream.str();
    data.assign( str.begin(), str.end() );
}

You can also avoid the std::string by using std::istreambuf_iterators:

void foo(std::vector<char> &data, std::stringstream &stream) {
    data.assign(
        std::istreambuf_iterator<char>( stream ), std::istreambuf_iterator<char>()
    );
}

but given that those are input iterators, the vector has no chance to know how much data will be assigned and could perform a bit worse, as it cannot reserve enough space to avoid reallocations.

K-ballo
  • 80,396
  • 20
  • 159
  • 169
  • 1
    I think you would probably want `istreambuf_iterator` here rather than `istream_iterator`, otherwise you'll lose whitespace. – ildjarn May 30 '12 at 20:04
  • Yes, aside from not specifying the character type. :-] – ildjarn May 30 '12 at 20:07
  • @ildjarn: Is it needed? The reference I'm checking says it defaults to `char`. Indeed, is needed. Seems like a quirk in that particular reference. – K-ballo May 30 '12 at 20:08
  • §24.6.3 says the signature is `template > class istreambuf_iterator`, no default. – ildjarn May 30 '12 at 20:09
  • You can obtain the correct size to `reserve` through `seekg` and `tellg` on the `std::stringstream`, see my answer. – Xeo May 30 '12 at 20:10
  • 1
    Oh btw, `istreambuf_iterator`s are constructible from streams, no need for the call to `rdbuf`. :) – Xeo May 30 '12 at 20:16
10

Your method invokes undefined behaviour. stream.str() returns a string by-value, aka a temporary string. You take the begin iterator of one temporary and the end iterator of the other, creating an invalid range.

One method to convert a stream to a container is to use the common iterator interface:

#include <iostream>
#include <sstream>
#include <vector>
#include <algorithm>
#include <iterator>

int main(){
  std::stringstream src("....");
  std::vector<char> dest;
  // for a bit of efficiency
  std::streampos beg = src.tellg();
  src.seekg(0, std::ios_base::end);
  std::streampos end = src.tellg();
  src.seekg(0, std::ios_base::beg);
  dest.reserve(end - beg);

  dest.assign(std::istreambuf_iterator<char>(src), std::istreambuf_iterator<char>());

  std::copy(dest.begin(), dest.end(), std::ostream_iterator<char>(std::cout));
}

Live example on Ideone.

Another method would be to cache the returned std::string object:

std::string const& s = stream.str();
data.reserve(s.size());
data.assign(s.begin(), s.end());
Xeo
  • 129,499
  • 52
  • 291
  • 397
  • Why are there two std::copy() calls? They seem redundant to me. – WilliamKF May 30 '12 at 20:26
  • @WilliamKF: The first (now replaced by `.assign`) was to insert into the vector, the second outputs to `std::cout` (too lazy for a handwritten loop). – Xeo May 30 '12 at 20:28
  • I suppose it's too much to hope that `src.rdbuf()->in_avail()` gives the correct size without messing around with `seek`? I'm not good with the stream libraries. – Steve Jessop May 30 '12 at 22:43
  • @Steve: You're better than me, it seems, since I didn't even remember that function until now. No idea if it will work the same, though. – Xeo May 30 '12 at 22:51
4

Copy from a stream iterator to a back insert iterator:

std::istream src;
std::vector<char> dst;

std::copy(std::istream_iterator<char>(src), std::istream_iterator<char>(), std::back_inserter(dst));

The istream_iterator uses formatted conversion (i.e. skips whitespace), so this may not be what you want. I'm not sure what your goal is.

brewbuck
  • 887
  • 6
  • 9
0

if there is a more efficient way

You may want to call reserve on your data and use the range insert member directly on data instead of using a copy-assignment. The thing about vectors that you need to remember is that every node may potentially increase the size (and relocate all elements). So, you're better off allocating the memory in one go (if you know how many objects you will be storing -- which you do know here) and leverage this fact.

dirkgently
  • 108,024
  • 16
  • 131
  • 187
  • 1
    I actually would assume most implementations will use a [`std::distance`](http://en.cppreference.com/w/cpp/iterator/distance) call in the iterator-based constructor to do that logic themselves. – KillianDS May 30 '12 at 20:06
  • 2
    @KillianDS : Likely only for random-access iterators, otherwise the range would be needlessly traversed twice. – ildjarn May 30 '12 at 20:22
  • @ildjarm: and necessarily not for input iterators, which cannot be traversed twice. – Steve Jessop May 30 '12 at 22:38