15
\$\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\$
23
\$\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)
{
    ifstream ifs(filename, ios::binary|ios::ate);
    ifstream::pos_type pos = ifs.tellg();

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

    ifs.seekg(0, 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\$
  • 1
    \$\begingroup\$ I love this answer, never thought of referencing vector[0] to access the raw memory block underneath it. \$\endgroup\$ – Jason Larke Feb 20 '13 at 7:53
  • 1
    \$\begingroup\$ This is very typical when using C++ containers and calling C code. \$\endgroup\$ – Martin York Feb 20 '13 at 16:20
  • 6
    \$\begingroup\$ An alternative to &vector[0] is vector.data(). \$\endgroup\$ – Aidiakapi Jul 18 '15 at 17:38
  • 1
    \$\begingroup\$ What about error checking. Both opening and reading might fail. \$\endgroup\$ – Zitrax Sep 23 '15 at 7:37
  • 1
    \$\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

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.