Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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

Can you tell whether that function 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;
}
share|improve this question

1 Answer 1

up vote 6 down vote accepted

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)
share|improve this answer
1  
I love this answer, never thought of referencing vector[0] to access the raw memory block underneath it. –  Jason Larke Feb 20 '13 at 7:53
1  
This is very typical when using C++ containers and calling C code. –  Loki Astari Feb 20 '13 at 16:20
    
I typically used the new unsigned char[size] approach, which meant I had to ensure the cleanup afterwards. This is much nicer. –  Jason Larke Feb 21 '13 at 3:06
    
Thank you, it's much simpler and cleaner. I thought of using array<char> but since it is (from what I've read) allocated on the stack it was not a good idea as my data wouldn't necessarily be small. Well, I've forgot to apply this again : "premature optimization is the root of all evil" ... –  Aybe Feb 21 '13 at 12:12

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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