How to do “try/finally” in C++ when RAII is not po

问题:

I'm coming back to C++ from a heavy C# background and I've inherited some C++ codebase which I think might not have been in line with the best C++ practices.

For example, I'm dealing with the following case (simplified):

// resource
class Resource {
 HANDLE _resource = NULL;
 // copying not allowed
 Resource(const Resource&); 
 Resource& operator=(const Resource& other);
public:
 Resource(std::string name) { 
  _resource = ::GetResource(name); if (NULL == _resource) throw "Error"; }
 ~Resource() {
  if (_resource != NULL) { CloseHandle(_resource); _resource = NULL; };
 }
 operator HANDLE() const { return _resource; }
};

// resource consumer
class ResourceConsumer {
 Resource _resource;
 // ...
 public:
  void Initialize(std::string name) {
   // initialize the resource
   // ...
   // do other things which may throw
 }
}

Here ResourceConsumer creates an instance of Resource and does some other things. For some reason (outside my control), it exposes Initialize method for that, rather than offering a non-default constructor, what apparently violates the RAII pattern. It's a library code and the API can't be refactored without making it a breaking change.

So my question is, how to code Initialize correctly in this case? Is it an acceptable practice to use an in-pace construction/destruction and a re-throw, like below? As I said, I came from C# where I'd simply use try/finally or the using pattern for that.

 void ResourceConsumer::Initialize(std::string name) {
  // first destroy _resource in-place      
  _resource.~Resource();
  // then construct it in-place
  new (&_resource) Resource(name);
  try {
    // do other things which may throw
    // ...
  }
  catch {
    // we don't want to leave _resource initialized if anything goes wrong
    _resource.~Resource();   
    throw;
  }
}

回答1:

Make Resource a moveable type. Give it move construction/assignment. Then, your Initialize method can look like this:

void ResourceConsumer::Initialize(std::string name)
{
    //Create the resource *first*.
    Resource res(name);

    //Move the newly-created resource into the current one.
    _resource = std::move(res);
}

Note that in this example, there is no need for exception handling logic. It all works itself out. By creating the new resource first, if that creation throws an exception, then we keep the previously created resource (if any). That provides the strong exception guarantee: in the event of an exception, the state of the object is preserved exactly as it was before the exception.

And note that there is no need for explicit try and catch blocks. RAII just works.

Your Resource move operations would be like this:

class Resource {
public:
    Resource() = default;

    Resource(std::string name) : _resource(::GetResource(name))
    {
        if(_resource == NULL) throw "Error";
    }

    Resource(Resource &&res) noexcept : _resource(res._resource)
    {
        res._resource = NULL;
    }

    Resource &operator=(Resource &&res) noexcept
    {
        if(&res != this)
        {
            reset();
            _resource = res._resource;
            res._resource = NULL;
        }
    }

    ~Resource()
    {
        reset();
     }

    operator HANDLE() const { return _resource; }

private:
    HANDLE _resource = NULL;

    void reset() noexcept
    {
        if (_resource != NULL)
        {
            CloseHandle(_resource);
            _resource = NULL;
        }
    }
};

回答2:

I'm leaving this answer here just for reference, as an example to an answer that didn't seek hard enough into the OP's full scenario. As the OP themselves rethrew the exception and clearly just used the try/catch clause for alleged RAII purposes, having no other use for it.

Nicol Bolas's answer is definitely the way to go.

Original answer:

If all you want to make sure is that the destructor for _resource is being called in case anything goes wrong, then you could have Resource _resource some unique smart-pointer, and then make a temporary smart-pointer in the scope of ResourceConsumer::Initialize() and eventually move the temp to _resource if all goes well. In all other scenarios the scope will be exited before the move and stack unwinding will call the appropriate destructor for the temporary.

Code example, in an attempt to stick as much as possible to your snippet in the question:

// resource consumer
class ResourceConsumer {
 template<class T> using prop_ptr = std::experimental::propagate_const<std::unique_ptr<T>>;
 prop_ptr<Resource> _resource;

 // ...
 public:
  void Initialize(std::string name);
};

void ResourceConsumer::Initialize(std::string name) {
  // first destroy _resource in-place      
  std::experimental::get_underlying(_resource).reset(); // See 'Note 2' below.

  // then construct it in-place
  auto tempPtr = std::make_unique<Resource>(name);
  // do other things which may throw
  // ...

  // Initialization is done successfully, move the newly created one onto your member
  _resource = move(tempPtr);

  // we don't want to leave _resource initialized if anything goes wrong
  // Fortunately, in case we didn't get here, tempPtr is already being destroyed after the next line, and _resource remains empty :-)
}

Note 1: Since I realized the catch clause was just rethrowing, we are getting the same effect without it just fine.

Note 2: You can safely remove the call to reset() in case you want your exception semantics to be such that in case of failed initialization then no change is made to resource. This is the preferable way, a.k.a. strong exception guarantee. Otherwise leave it there to guarantee empty resource in case of initialization failure.

Note 3: I'm using a propagate_ptr wrapper around unique_ptr to preserve const-qualification of _resource member under const access path, i.e. when working with a const ResourceConsumer. Don't forget to #include <experimental/propagate_const>.