Is there a more efficient way to set a std::vector from a stream?

Question

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?

Answer 1

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());

Answer 2

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.

Answer 3

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.

Answer 4

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.