8
\$\begingroup\$

I need to read a range of bytes from a file and return it as a std::vector<std::byte>.

My current code looks like this:

#include <cstdint>
#include <fstream>
#include <string>
#include <vector>

using bytes = std::vector<std::byte>;
using namespace std;

bytes read_block(uint32_t offset,
                 uint32_t length,
                 const string& filename) {
  ifstream is(filename, ios::binary);
  is.seekg(offset);
  byte data[length];
  is.read(reinterpret_cast<char*>(data), length);
  return bytes(data, data + length);
}

But as can be seen I read it into a byte array and then create the vector by copying the bytes in the constructor. I have some issues with this implementation:

  • Copying of bytes when creating output variable.
  • Use of C style array.
  • Use of casts.

I have left out error handling for now to focus on the specific issues above.

\$\endgroup\$
  • \$\begingroup\$ Creating byte data[length]; then creating a vector forces a copy. Why not make data a vector and simply return it. Then the compiler can do lots of optimization and build the vector directly at the destination so removing the need for a copy. \$\endgroup\$ – Martin York Jun 11 at 13:48
8
\$\begingroup\$

Nice:

using bytes = std::vector<std::byte>;

I would call it Bytes to make it clear it is a type rather than an object.


This is not a good idea:

using namespace std;

See: Why is “using namespace std;” considered bad practice?


This is technically not valid C++.

  byte data[length];

Variable sized arrays (VSA) are an extension to the language supported by several compilers but not actually part of the C++ standard. Though it is part of the more recent versions C standard.

Also creating a local array means that the data is being created on the local stack frame (here I assuming a Von-Neumann like architecture). This means for the data to exist after the function completes means you will need to copy it out of the function. So an alternative data object is probably a good idea.

Also there is a limitation on the size of array allowed on the stack (there are hardware limitations on the size of stack frames on lots of architecture).


So a god idea would be to use the same data structure that you return.

  Bytes data(length);   // This is basically std::vector

Now in older versions of C++ there are some nice optimizations that can be applied to std::vector when it is returned from a function that allow the compiler to build the std::vector at the final destination so that it is not actually copied on return.

In modern versions of the compiler the language has the concept of move semantics built into the language. So returning a std::vector is not an expensive operation as the internal buffer is moved (just the pointer is copied) from the function to the destination result.

Also the compiler is now required to perform copy elision of objects when it can.


Error checking

This code does zero error checking. There are several things that can go wrong and you don't check for any of them. But you said you removed this (you should have left it in).


So a better version:

#include <vector>
#include <string>
#include <fstream>
#include <cstdint>

using Bytes = std::vector<std::byte>;

Bytes read_block(std::uint32_t      offset,
                 std::uint32_t      length,
                 std::string const& filename)
{
  // Not going to explicitly check these.
  // The use of gcount() below will compensate for a failure here.
  std::ifstream is(filename, std::ios::binary);
  is.seekg(offset);

  Bytes data(length);
  is.read(reinterpret_cast<char*>(data.data()), length);

  // We have to check that reading from the stream actually worked.
  // If any of the stream operation above failed then `gcount()`
  // will return zero indicating that zero data was read from the
  // stream.
  data.resize(is.gcount());

  // Simply return the vector to allow move semantics take over.
  return data;
}
\$\endgroup\$
1
\$\begingroup\$

I played around with this a bit more and came up with some alternatives.

First I tried to use basic_ifstream<byte> like this:

bytes read_block(uint32_t offset,
                 uint32_t length,
                 const string& filename) {
  basic_ifstream<byte> is(filename, ios::binary);
  istreambuf_iterator<byte> it(is);
  bytes data;
  copy_n(next(it, offset), length, data.begin());
  return data;
}

this compiles without issues and does not use C style arrays nor casts and copies directly from the file to the output vector (thus solving all of my mentioned issues).

However this throws a std::bad_cast at runtime. Based on this it might be due to stl not having a char_traits for std::byte. Annoying that it's a runtime issue and not a compile issue. As of now I don't know how or if it's simple/possible to implement a char_traits for std::byte myself.

My next version loosens up on the cast restriction, but works also at runtime:

bytes read_block(uint32_t offset,
                 uint32_t length,
                 const string& filename) {
  ifstream is(filename, ios::binary);
  is.seekg(offset);
  bytes data;
  data.resize(length);
  is.read(reinterpret_cast<char*>(data.data()), length);
  return data;
}
\$\endgroup\$

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.