views:

179

answers:

5

Hi, I have this function:

char* ReadBlock(fstream& stream, int size)
{
    char* memblock;
    memblock = new char[size];
    stream.read(memblock, size);
    return(memblock);
}

The function is called every time I have to read bytes from a file. I think it allocates new memory every time I use it but how can I free the memory once I have processed the data inside the array? Can I do it from outside the function? Processing data by allocating big blocks gives better performance than allocating and deleting small blocks of data?

Thank you very much for your help!

+1  A: 

Just delete[] the return value from this function when you've finished with it. It doesn't matter that you're deleting it from outside. Just don't delete it before you finish using it.

It has to be `delete[]`, not `delete`.
Georg Fritzsche
Ouch! The sad thing is that if you make this mistake, the compiler can't even detect it. What an awful language. Anyway, thanks. I'll fix the post. (Though it must be said, I wasn't actually proposing the entire line of code, just what builtin to use.)
+1  A: 

Yes. You may call delete from outside of the function. In this case though, may I suggest using an std::string so you don't have to worry about the management yourself?

Clark Gaebel
Ok, but I cannot use strings because of read. istream
emerrf
Then use a vector instead. Sorry, didn't notice that.
Clark Gaebel
+3  A: 

You can call:

char * block = ReadBlock(stream, size);
delete [] block;

But... that's a lot of heap allocation for no gain. Consider taking this approach

char *block = new char[size];
while (...) {
  stream.read(block, size);
}
delete [] block;

*Note, if size can be a compile time constant, you can just stack allocate block.

Stephen
Solid call on passing a static pointer into the function. +1
Platinum Azure
Variable sized arrays aren't ISO C++, it's a GCC extension.
Clark Gaebel
@wowus : Who says size isn't compile time?
Stephen
The definition of the function says that: char* ReadBlock(..., int size);
Clark Gaebel
@wowus : This would obviate the need for the function, anyways, point taken... commented.
Stephen
Wouldn't I run out of memory if I don't delete already read memory blocks?
emerrf
Stephen, why has this become `new int[size]` instead of `new char[size]`?
Georg Fritzsche
@Georg : Hmmm, good question. Thanks fixed. ;)
Stephen
@emerrf : You can reuse the same buffer, if your application allows for it (whether that's true isn't clear from your question). If it 's not possible to reuse the same buffer, then you should use a different approach - such as returning a string.
Stephen
+10  A: 

Dynamic arrays are freed using delete[]:

char* block = ReadBlock(...);
// ... do stuff
delete[] block;

Ideally however you don't use manual memory management here:

std::vector<char> ReadBlock(std::fstream& stream, int size) {
    std::vector<char> memblock(size);
    stream.read(&memblock[0], size);
    return memblock;
}
Georg Fritzsche
With this method, when the vector object is returned, does its copy constructor get called (I mean, do two instances exist at one point)?
dreamlax
@dream: It depends, most likely *named return value optimization*, [NRVO](http://en.wikipedia.org/wiki/Return_value_optimization), will kick in. See also the interesting [Want Speed? Pass by value.](http://cpp-next.com/archive/2009/08/want-speed-pass-by-value/).
Georg Fritzsche
@Georg : Interesting post, but _very_ misleading name. It almost sounds as if they're arguing pass-by-value instead of pass-by-reference. Link bait, I guess.
Stephen
If it is not as trivial as the above example, I would always use BOOST shared_ptr (or intrusive_ptr if the type of data is a class). It may sound heavy weight but leads to consistent leak free code (almost).
hackworks
@hack: Then `shared_array<T>` would be a better fit here, no need for a custom deleter.
Georg Fritzsche
@Georg: Ah I see! So, if I read correctly, the standard permits an optimisation that prevents a copy constructor from being called even if the copy ctor has side effects?
dreamlax
@dream: Yep, its a notable exception to the rule of not changing the observable behaviour. RVO is e.g. observable with logging-statements in both ctor and copy-ctor with `X f() { return X(); } X x = f();` (activated optimizations assumed). If you have a standard handy see *§12.8/15*.
Georg Fritzsche
+1  A: 

first thing to note: memory allocated with new and delete is completely global. things are not automatically deleted when pointers go out of scope or a function is exited. as long as you have a pointer to the allocation (such as the pointer being returned there) you can delete it when ever and where ever you want. the trick, is just makeing sure other stuff doesn't delete it with out you knowing.

that is a benefit with the sort of function structure the fstream read function has. it is fairly clear that all that function is going to do is read 'size' number of bytes into the buffer you provide, it doesn't matter whether that buffer has been allocated using new, whether its a static or global buffer, or even a local buffer, or even just a pointer to a local struct. and it is also fairly clear that the function is going to do nothing more with the buffer you pass it once it's read the data to it.

on the other hand, take the structure of your ReadBlock function; if you didn't have the code for that it would be tricky to figure out exactly what it was returning. is it returning a pointer to new'd memory? if so is it expecting you to delete it? will it delete it it's self? if so, when? is it even a new pointer? is it just returning an address to some shared static buffer? if so, when will the buffer become invalid (for example, overwritten by something else)

looking at the code to ReadBlock, it is clear that it is returning a pointer to new'd memory, and is expecting you to delete it when ever you are done with it. that buffer will never be overwritten or become invalid until you delete it.

speed wise, thats the other advantage to fsream.read's 'you sort out the buffer' approach: YOU get the choice on when memory is allocated. if you are going "read data, process, delete buffer, read data process delete buffer, ect.... " it is going to be alot more efficient to just allocate one buffer (to the maximum size you will need, this will be the size of your biggest single read) and just use that for everything, as suggested by Stephen.

matt