1

I have the following code:

void do_something(Image *image)
{

    Image *smoothed = NULL;
    Image *processed = NULL;
    if (condition_met) {
        smoothed = smooth(image);
        processed = smoothed;
    }
    else {
        processed = image;
    }

    ...
    delete smoothed_image
}

I was wondering if I can do the following and if this is the correct way. I am confused about setting another pointer from an auto_ptr object and whether that changes the ownership somehow.

void do_something(Image *image)
{
    auto_ptr<Image *> smoothed;
    Image *processed = NULL;

    if (condition_met) {
        smoothed = smooth(image); // This should own the pointer returned by smooth
        processed = smoothed; // Is this OK? processed is not an auto_ptr
    }
    else {
        processed = image;
    }

    ...
    // Destructor for the 'smoothed' variable should be called.
    // The 'image' variable is not deleted.
}

Will the destructor get called as I intend it and is this the correct way to do this?

Luca
  • 10,458
  • 24
  • 107
  • 234
  • 5
    it should be `auto_ptr smoothed;` and you should probably use `smoothed.reset(new_value);` to set and `smoothed.get()` to get the pointer that is owned by the `auto_ptr`. Also, `auto_ptr` should not be used anymore if at all possible, if you can use `unique_ptr` instead. – PeterT Nov 19 '14 at 11:54
  • 2
    @Luca Where is smoothed_image declared? – Vlad from Moscow Nov 19 '14 at 11:58
  • @PeterT why write an answer as a comment? – Kos Nov 19 '14 at 12:01
  • @Vlad: I made the correction. – Luca Nov 19 '14 at 12:06
  • @Peter: Thanks for the suggestions. I am using auto_ptr as I am not sure of using C++ 11 features yet in my code but you are right, I will make a switch to unique_ptr at some point. – Luca Nov 19 '14 at 12:06
  • 1
    @Luca even using `boost::unique_ptr` in non-C++11 code seems like a better solution. The semantics of `auto_ptr` are confusing. "Move on copy" is just a ridiculous thing to do. – PeterT Nov 19 '14 at 12:09
  • @PeterT The practical semantics of `std::auto_ptr` are almost the same as those of `std::unique_ptr`; they both do a form of "move on copy" (which while somewhat surprising at first, is very practical in some cases). – James Kanze Nov 19 '14 at 12:13
  • 1
    @JamesKanze well, you simply can't copy a `std::unique_ptr`, so to move it, you need to actually use move-semantics. – PeterT Nov 19 '14 at 12:15
  • @PeterT Yes. But in almost all of the cases where you'd actually use an `std::unique_ptr`, the move semantics will be automatic (or not used at all---in a lot of cases, `std::unique_ptr` or `std::auto_ptr` are never moved or copied at all). If you're sure that all targeted compilers support `std::unique_ptr`, by all means use it; it's future safe. Otherwise, however, using `std::auto_ptr` is no big deal. – James Kanze Nov 19 '14 at 12:22
  • 1
    @JamesKanze: No, `unique_ptr` does a "move on move", which is just as practical but not surprising. – Mike Seymour Nov 19 '14 at 12:22
  • 1
    @JamesKanze Oh, I agree, if it's just in some local scope, it's not a big deal (but even there you could copy it twice by passing it as a parameter to 2 functions or something, so why take the risk, when there's no reason to). But especially when you're starting to make it a data-member or a template parameter of a container, then the semantics are waaaay surprising. – PeterT Nov 19 '14 at 12:24
  • Unfortunately, using boost is not an option for my work :/ – Luca Nov 19 '14 at 18:12

1 Answers1

3

A few points to make. Assuming that the signature of the smooth function is 

Image* smooth(Image*);

Then your code would minimally have to be changed to

void do_something(Image *image)
{
    auto_ptr<Image> smoothed;
    Image *processed = NULL;

    if (condition_met) {
      smoothed.reset(smooth(image)); // This should own the pointer returned by smooth
      processed = smoothed.get(); // Maybe?  Depends on what you're doing
    }
    else {
      processed = image;
    }

It's reasonable in some cases to pull the raw pointer out of a smart pointer as is done in by smoothed.get() above, but you have to understand that as written the pointer held by smoothed will be deleted at the end of the function, even though you've done something else with the raw pointer. Not enough information here to see if that's a problem, but it's a smell.

std::auto_ptr is now deprecated in favor of std::unique_ptr. The big reason for this is the way the contents of auto_ptr are moved:

std::auto_ptr<int> a = new int(5);
std::auto_ptr<int> b = a;  // Copy?  No!

In this code the pointer held by a has been transferred to b. a is no longer holding anything. This runs counter to how we usually think of copy behavior so it's easy to mess up.

C++11 introduced the concept of r-value references (there are loads of articles around the net explaining this, including What are move semantics?). Having r-value references allows unique_ptr to prevent data from being moved where it doesn't make sense.

std::unique_ptr<int> a = new(5);
std::unique_ptr<int> b = a; // Compile fail.  Not copyable
std::unique_ptr<int> c = std::move(a); // OK, shows intent

With this available, it would be possible to update your smooth() function signature to

std::unique_ptr<Image> smooth(Image*) {
  ...
  return newImage;
}

Which clearly instructs the caller that they should take ownership of of the returned pointer. Now you can say

unique_ptr<Image> smoothed;
...
smoothed = smooth(image);

Because a value returned from a function is an r-value (not bound to a variable yet), the pointer is safely moved to smoothed.

In the end, yes, using a smart pointer is preferable to calling delete. Just think through your design and try to be clear about pointer ownership.

Community
  • 1
  • 1
Jay Miller
  • 2,184
  • 12
  • 11
  • How about linking to the [c++-faq] entry on [move semantics here on SO](http://stackoverflow.com/q/3106110/256138)? – rubenvb Nov 19 '14 at 15:28