16
\$\begingroup\$

I'm basically trying to write a helper function that reads a whole file and returns the data and the number of bytes read.

Can you tell me if is correctly written and used?

#include <iostream>

static char * ReadAllBytes(const char * filename, int * read)
{
    ifstream ifs(filename, ios::binary|ios::ate);
    ifstream::pos_type pos = ifs.tellg();
    int length = pos;
    char *pChars = new char[length];
    ifs.seekg(0, ios::beg);
    ifs.read(pChars, length);
    ifs.close();
    *read = length;
    return pChars;
}

int _tmain(int argc, _TCHAR* argv[])
{
    const char * filename = "polar00.map";
    int read ;
    char * pChars = ReadAllBytes(filename, &read);
    delete[] pChars;
    return 0;
}
\$\endgroup\$
27
\$\begingroup\$

A few things I would do differently:

static char * ReadAllBytes(const char * filename, int * read)
{
    ifstream ifs(filename, ios::binary|ios::ate);
    ifstream::pos_type pos = ifs.tellg();

    // What happens if the OS supports really big files.
    // It may be larger than 32 bits?
    // This will silently truncate the value/
    int length = pos;

    // Manuall memory management.
    // Not a good idea use a container/.
    char *pChars = new char[length];
    ifs.seekg(0, ios::beg);
    ifs.read(pChars, length);

    // No need to manually close.
    // When the stream goes out of scope it will close the file
    // automatically. Unless you are checking the close for errors
    // let the destructor do it.
    ifs.close();
    *read = length;
    return pChars;
}

How I would do it:

static std::vector<char> ReadAllBytes(char const* filename)
{
    std::ifstream ifs(filename, std::ios::binary|std::ios::ate);
    std::ifstream::pos_type pos = ifs.tellg();

    if (pos == 0) {
        return std::vector<char>{};
    }

    std::vector<char>  result(pos);

    ifs.seekg(0, std::ios::beg);
    ifs.read(&result[0], pos);

    return result;
}

Note:

static std::vector<char> ReadAllBytes(char const* filename)

It may seem like an expensive copy operation. But in reality NRVO will make this an in-place operation so no copy will take place (just make sure you turn on optimizations). Alternatively pass it as a parameter:

static void ReadAllBytes(char const* filename, std::vector<char>& result)
\$\endgroup\$
25
  • 2
    \$\begingroup\$ I love this answer, never thought of referencing vector[0] to access the raw memory block underneath it. \$\endgroup\$ Feb 20 '13 at 7:53
  • 1
    \$\begingroup\$ This is very typical when using C++ containers and calling C code. \$\endgroup\$ Feb 20 '13 at 16:20
  • 13
    \$\begingroup\$ An alternative to &vector[0] is vector.data(). \$\endgroup\$
    – Aidiakapi
    Jul 18 '15 at 17:38
  • 2
    \$\begingroup\$ What about error checking. Both opening and reading might fail. \$\endgroup\$
    – Zitrax
    Sep 23 '15 at 7:37
  • 3
    \$\begingroup\$ In C++11, you can also get to the vector's memory by calling vector.data(). \$\endgroup\$
    – Tolli
    Jun 28 '16 at 14:39
3
\$\begingroup\$

Missing error checking

Both your code and the code from Martin York's answer don't perform any error checking. Things can go wrong both when opening a file (it might not exist, you might not have the right permissions, and so on), and when reading from a file (less likely at this point, but read errors are still possible). You want to ensure two things:

  • Your program doesn't crash while trying to read a file.
  • Any errors are not silently ignored, as this might cause your program to give incorrect results without you knowing it.

I recommend checking that ifs.eof() == true after reading all the data. If it's not, then you didn't succesfully reach the end of the file, and then report the error in some way; for example, throw an exception or use a std::optional for the return value.

tellg() will return -1 in case of an error. When trying to allocate memory for the result, that -1 might be cast to an unsigned std::size_t and become a huge number, causing the memory allocation to fail. This might cause your program to crash. So check the return value of tellg() before using it.

\$\endgroup\$
3
\$\begingroup\$

I would amend Martin York's answer:

  • use std::filesystem::path (standard as of C++17; part of Boost before that) instead of std::string for the parameter.
  • use vector::data instead of taking the address of the first element.

I'm also a little worried about using ate mode but then seeking back to the beginning. Is this guaranteed to only jump back to the end on any write operation, but allow me to read from any position? Also, what if the file is read-only? I'd rather open for reading only, not specify that I'll be writing to the end because I don't need write access at all!

Reading the docs, I see that ate just seeks right after opening, as distinct from app which forces writes on the end (even though it stands for append to end). It can be combined with other flags without otherwise affecting the meaning. So, use

std::ios::in|std::ios::binary|std::ios::ate

\$\endgroup\$
2
  • \$\begingroup\$ Don't be too harsh on that first point - that std::filesystem didn't exist in 2013. It's a good suggestion for updating to make it more type-safe in modern contexts, though. :-) \$\endgroup\$ Oct 14 '21 at 6:59
  • \$\begingroup\$ And std::ios::ate ("open at e ​nd") doesn't imply writing at all - and std::ifstream constructor always includes std::ios::in, so no need to redundantly specify that. \$\endgroup\$ Oct 14 '21 at 7:01
2
\$\begingroup\$

I would wrap the native memory-mapped file functionality instead. It’s available on all major platforms. In all cases it will be more efficient than any “read the whole file” code would be. If you absolutely need to keep the file’s contents yet close the file, you can then initialize a vector from the memory-mapped byte range, and close the range afterwards. It will still be faster and less cumbersome than reading the file.

Typically, you won’t have to bother with errors once the file is open and mapped: read errors become page faults. You can of course trap those if needed, but if there’s no viable way for the application to proceed if the file cannot be read after successfully mapping it, then terminating is a good option. But, to be flexible, I’d ensure that while the file is mapped, any I/O errors that turned into page faults get caught and handled appropriately per the preference of the user of your code.

\$\endgroup\$
4
  • \$\begingroup\$ Boost provides an abstraction of memory-mapped files, so if that's available to you, there's no need to write your own code for the various APIs. \$\endgroup\$ Oct 12 '21 at 16:54
  • \$\begingroup\$ On 32-bit systems you can only map about 2 GB this way. Also, for small files, this might actually be less efficient, as setting up the page tables and handling the initial page faults when accessing the mapped range for the first time isn't free. \$\endgroup\$
    – G. Sliepen
    Oct 12 '21 at 17:59
  • \$\begingroup\$ If the normal handle-based file I/O is implemented in terms of memory mapping for local files, then the page faults and page tables etc. is not any worse than it does anyway. The big deal is the overhead of keeping an open file if it's a small amount of data. \$\endgroup\$
    – JDługosz
    Oct 13 '21 at 15:27
  • \$\begingroup\$ If the file to be slurped is not on the local file system, memory mapping may not work or be very inefficient. So you need a fallback for reading remote files anyway. \$\endgroup\$
    – JDługosz
    Oct 13 '21 at 15:28

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.