14
\$\begingroup\$

I need to read n chars from a binary file into a string. Currently, what I do is:

static string Read(istream &stream, uint32_t count)
{
    auto bytes = unique_ptr<char[]>(new char[count]);

    stream.read(bytes.get(), count);

    return string(bytes.get(), count);
}

I found the way I deal with the array of chars quite messy. If I used new and delete[] directly, it would make the code messy in another way (I would need to add a local variable for the result). And I'm trying to avoid delete as a general rule.

Is there a clear way to write this code? The fact that it uses twice as much memory as it needs is probably not a big deal, but fixing that would be nice too.

\$\endgroup\$
  • \$\begingroup\$ I think this is actually quite clean – albeit low-level – code. Nothing really wrong with it, and it’s probably more efficient than ruds’ implementation, although that’s arguably even cleaner. \$\endgroup\$ – Konrad Rudolph Jul 20 '13 at 13:51
  • \$\begingroup\$ @KonradRudolph: There is no need to be calling new here. Especially since the string object will deal with all that for you. \$\endgroup\$ – Martin York Jul 20 '13 at 21:48
  • \$\begingroup\$ @Loki Ah true, I’d completely forgotten about C++11’s contiguity requirement. \$\endgroup\$ – Konrad Rudolph Jul 20 '13 at 22:03
19
\$\begingroup\$

Why not:

static string Read(istream &stream, uint32_t count)
{
    std::string result(count, ' ');
    stream.read(&result[0], count);

    return result;
}

Though not strictly C++03 compatible that is easily validated. One of the reasons the committee found it easy to add the new constraint in C++11 was that no implementation did not use contiguous memory (Random Access Iterators are a hint).

But a C++03 strict implementation would be:

static string Read(istream &stream, uint32_t count)
{
    std::vector<char> result(count);  // Because vector is guranteed to be contiguous in C++03
    stream.read(&result[0], count);

    return std::string(&result[0], &result[count]);
}
\$\endgroup\$
  • \$\begingroup\$ Yeah, that's certainly better. \$\endgroup\$ – svick Jul 20 '13 at 21:57
  • 3
    \$\begingroup\$ Note that this requires C++11 in a non-obvious way (C++03 didn't require that &result[0] worked this way). Otherwise, LGTM. \$\endgroup\$ – ruds Jul 20 '13 at 22:12
  • \$\begingroup\$ Depending on your use case, you may want to do result.resize(stream.gcount());, in case not all of count bytes could be read. \$\endgroup\$ – Josh Kelley Apr 21 '15 at 18:40
8
\$\begingroup\$

First of all, it seems that you've got a using namespace std; somewhere in your code. Don't do that. (No, really).

Here's a function that should meet your needs.

static std::string Read(std::istream &stream, std::string::size_type count)
{
    std::string out;
    out.reserve(count);
    std::copy_n(std::istreambuf_iterator(stream), count, std::back_inserter(out));
    return out;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ uint32_t should have an std:: as well. \$\endgroup\$ – Jamal Jul 19 '13 at 20:07
  • \$\begingroup\$ I don't have using namespace std; there. Instead, I have using std::uint32_t; using std::unique_ptr; using std::string; using std::istream; there. It's kind of annoying, but I thought it would be better than writing std:: everywhere. \$\endgroup\$ – svick Jul 19 '13 at 20:12
  • 3
    \$\begingroup\$ @Jamal You're right. Actually, I've changed it to std::string::size_type, as that's the size class we're interested in. \$\endgroup\$ – ruds Jul 19 '13 at 20:13
  • \$\begingroup\$ @svick I think it's odd to read code with names from std but with no std:: in front. Personal tastes vary, of course. \$\endgroup\$ – ruds Jul 19 '13 at 20:14
  • \$\begingroup\$ Is this going to insert the characters one by one? I worry that doing that would be too slow (the result might be one megabyte or so). (Of course, the proper way to find that out would be to measure it.) \$\endgroup\$ – svick Jul 19 '13 at 20:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.