This is a long one. I have a function that loads some image data from disk. I've tried this with three different methods, one of which doesn't work, and I'm wondering what the smartest method is.

Method 1:

I used to have it set up so that the loaded data was an argument to the function, which seemed to require allocating the space for it outside the function then passing a pointer to the allocated space. This requires knowing the size of the image ahead of time, so a helper function had to be made to get the image width and height first. The following is an example.

Pros: More explicit in the calling code that memory is being allocated and therefore should be deleted.

Cons: Need to know image size ahead of time.

// Data allocated outside the image, allocated space passed to function. This works.
// Notice that width & height are passed to the function. 
size=(*width)*(*height);
image  = new unsigned char[size];

void read_pgm(unsigned char *image, char *file_name, int width, int height){

    // Code to read sizeof(char)*width*height bytes of data from the file into image

}

Method 2:

I thought it would be nice to have the function allocate its own data so I don't need to pass it a size. If I tried to allocate the space for it in the function, it seemed to be lost after the function ended. In the following case, the function read_pgm runs fine, but if I then try to write that data to another file, my code crashes.

Pros: Don't need to know image size ahead of time, no need to allocate data in calling code.

Cons: Doesn't work. Also, if it did, would calling it repeatedly in a loop result in a memory leak if I wasn't clearing image outside the function?

// Data allocated inside the image for a pointer passed to the function. This doesn't work.
void read_pgm(unsigned char *image, char *file_name, int *width, int *height){

    size=(*width)*(*height);
    image  = new unsigned char[size];

    // Code to read the data from the file into image

}

Method 3:

Here, the data is again allocated in the function but is handed back as a returned item. This works, i.e. I can then write the data out to another image fine. I don't understand why this works and Method 2 does not.

Pros: Same as Method 2.

Cons: Same as Method 2 except th开发者_开发百科is currently works.

// Data allocated in the function, and returned. This works.
unsigned char* read_pgm(char *file_name, int *width, int *height){

    // Allocate data for the image
    size=(*width)*(*height);
    image  = new unsigned char[size];

    // Code to read the data from the file into image

    return image; // Return pointer to the data
}

So, my questions are:

1. In a case like this, is it smarter to set the function up to allocate space itself so the calling code doesn't need to supply the image size? Or is it smarter to allocate outside the function as a reminder that delete needs to be called on image at some point. Or am I wrong in thinking that this needs to be done? It seems like calling Method 2 or Method 3 in a loop would make a memory leak.

2. Why doesn't Method 2 work?

Thanks.

If you want to know the smart way, then the answer has to be, "None of the above". The smart way? Use a vector. That's what it's for. Because using new directly sucks. Managing the bounds of your own memory sucks. We have classes for that. And char* for string? At least make it a const char*. const std::string& better. I also have to ask- what image format are you trying to read which doesn't store the width and height of the image in the file format? Seems to me like you'd be better off reading that from the file.

std::vector<unsigned int> void ReadImage(const std::string& filename, int width, int height) {
    std::vector<unsigned int> imageData(width * height);
    // Read here from filestream
}

std::vector<unsigned int> imageData = ReadImage("ohai.png", 1000, 600);

You need to learn about - const correctness, RAII, and the Standard library.

Answer to question 2

In order to make it work you must pass a reference to the pointer

unsigned char * & image

Otherwise you allocate memory and the COPY of the passed pointer points to it. The original pointer object doesn't change.

Answer to question 1

Ever heard of smart pointers?

Smart pointers can be used to manipulate memory all by themselves. If you don't want to use smart pointers for some inexplicable reason, such as pseudooptimization, then I think you described the pros and cons of all methods yourself - there's always tradeoff. Up to you to decide

Method 2 doesn't work because you are passing the pointer by value, and overwriting that value locally, so the value of the pointer outside the function isn't changed. You can fix this by either passing it by reference or by passing a pointer to the pointer.

As for the other question: it's really just a matter of clearly documenting who has got the responsibility to delete the allocated data. (By the way: yes, you should deallocate otherwise you will leak). In many APIs you will see in the documentation: "the caller has the responsiblity to delete this" or "call this other function to delete the allocated data".

A good way to decouple this kind of ownership concerns is to use smart pointers (as Armen suggests).

Everything you do currently results in a memory leak, since you never delete[] anything.

First off, yes, make the dimensions variable. Second, either return a pointer to the newly allocated heap storage and remember to delete it in the caller, or return a smart container object.

A std::vector<unsigned char> would do nicely:

std::vector<unsigned char> get_image(const std::string & filename, size_t & width, size_t & height)
{
  // determine width and height

  /* ... */

  std::vector<unsigned char> result(width * height, 0);

  // read into &result[0], vector guarantees contiguous storage

  return result;
}

Consider encapsulating all the functionality related to reading and "handling" the data in a class.

It should take the file path as a std::string in the constructor.

Creating the file handle, allocating the memory and reading can then be done in a separate function (something like Init). This gives you the ability to create the object when you know the file path but do the time-consuming part (the reading part) later, when you actually need it.

You can expose whatever information is necessary to the outside users (like the height or width).

The class destructor will be responsible for closing the file handle (which can be done earlier, if needed) and de-allocating the data.

You can use new and delete with your class or, better yet, use a smart pointer with your class and not just with the image data part. You can also use it "on the stack" and have its destructor called automatically when out of scope.

"2.Why doesn't Method 2 work?"

Because you're passing a pointer by copy, not by reference. You pass in a pointer, and then prompty reinitialize where that pointer points to with your new call. When the scope of the function is reached, so is the life of the local variable image, as well where all that lovely new storage went.

What you really want to do (in this case), is pass either a reference to the pointer or a pointer to the pointer so that you can change the value of the passed argument (this case a pointer), and have that change be visible outside the scope of the function.

void read_pgm(unsigned char **image, char *file_name, int *width, int *height)
{      
    size=(*width)*(*height);     
    (*image)  = new unsigned char[size];      

    // Code to read the data from the file into image  
} 

There's actually a really excellent section in the C++ FAQBook on these questions.

The big issue is, as you say, ensuring that the memory is allocated in a way that ensures it doesn't leak. C++ has lots of convenient facilities to do that, the most obvious being new and delete.

The easiest thing is to allocate the storate with new in a context where it will go out of scope. So...

 {
     MyData *md = new MyData(args);

     doSomething(md);

 }

now, when md goes out of scope, it will automagically have the dtor called. Now, this trick will work pretty much everywhere.

With your method 2, you're right that written as it is, it leaks memory; the next time you do a new the reference to that last time is lost, but it's not finalized or destroyed.

A solution is to explicitly call the dtor, ie, delete it, somewhere else.

I suspect the reason it doesn't work is that image is a pointer, which is what new wants to return. but since C++ does pointers call-by-value, you're putting that address into local memory on the stack, and it disappears.

You either want a **, or even better a reference.

A third solution is that C++ does have pointer types to help with free store management. The FAQbook describes how to implement reference-counting pointers here.