HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMajor

Reading all bytes from a file

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
readingfileallbytesfrom

Problem

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 

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;
}

Solution

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 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{};
    }

    std::vector  result(pos);

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

    return result;
}


Note:

static std::vector 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& result)

Code Snippets

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;
}
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;
}
static std::vector<char> ReadAllBytes(char const* filename)
static void ReadAllBytes(char const* filename, std::vector<char>& result)

Context

StackExchange Code Review Q#22901, answer score: 27

Revisions (0)

No revisions yet.