2

I have a class called Camera which opens a camera with v4l2_open, etc., in the constructor. The destructor does some cleaning and closes the file descriptor with v4l2_close.

When the camera crashes, what I do is to delete the object and then create a new one:

Camera *camera = new Camera();
(...)
if (crash) {
  delete camera;
  camera = new Camera();
}

Is this one of the correct uses of new/delete in C++?

João M. S. Silva
  • 1,078
  • 2
  • 11
  • 24
  • 3
    possible duplicate of [Why should I use a pointer rather than the object itself?](http://stackoverflow.com/questions/22146094/why-should-i-use-a-pointer-rather-than-the-object-itself) – E_net4 Feb 27 '15 at 17:23
  • 2
    Prefer using an object. If you must allocate dynamically, prefer using a smart pointer to an object on the heap. – Thomas Matthews Feb 27 '15 at 17:25
  • 2
    Why do people downvote questions like these? I don't get you guys sometimes. – Lawrence Aiello Feb 27 '15 at 17:29
  • @LawrenceAiello Probably because it's a duplicate. – Emil Laine Feb 27 '15 at 17:36
  • 1
    Well this site has been around for almost 7 years now. Almost every potential question someone has is a duplicate. Why don't we just shut SO down if that's how we're going to treat these? – Lawrence Aiello Feb 27 '15 at 17:40
  • 1
    @E_net4, I've read the top answers of the possible duplicate question. I'd prefer using an object for the therein mentioned reasons, but the problem here is the hardware failure during the C++ object's lifetime. A situation like this does not seem described in the answers of the possible duplicate. I don't think this is a simple object vs. pointer issue. – João M. S. Silva Feb 27 '15 at 17:44
  • 2
    Then why are you asking whether you should use an object or a pointer? Why not ask how to deal with a hardware error? – juanchopanza Feb 27 '15 at 17:45
  • 1
    @juanchopanza, When I searched the Internet I learned that you should use an object 99% of the time. I wanted to know if this was one of the 1% cases or if there was a better alternative. Why is this wrong? – João M. S. Silva Feb 27 '15 at 17:48
  • 1
    @JoãoM.S.Silva The title was so generic that I seriously missed the part that you were dealing with an interface to a camera hardware. Regardless, once you understand the lifetime of an object, you should be able to adjust such a particular case to suit your needs. – E_net4 Feb 27 '15 at 17:48
  • @E_net4, I changed the title to try to make it more informative. – João M. S. Silva Feb 27 '15 at 17:52
  • 2
    If you are going to use a pointer, use `std::unique_ptr` – Neil Kirk Feb 27 '15 at 17:56

1 Answers1

6

No, the use of new and delete is not warranted here. If your camera “becomes bad” and you wish to dispose of it in favor of a new one, simply assign a new one.

const std::string device {"/dev/cameras/front"};  // whatever
Camera camera {device};
// do something...
if (camera.bad())
  camera = Camera {device};  // replace by a new one

You'll probably want to overload the assignment operator of your Camera class for this to work. Since the Camera class is resource-owning, it should not be copyable but movable. I don't know how you are talking to the hardware so I've made the following example a bit up but it should give you the correct idea how to implement your type.

extern "C"
{
  // I have made these up...
  int camera_open(const char *);
  int camera_close(int);
}

class Camera
{

private:

  // Initially set to arbitrary nonsensical values.
  std::string device_ {};
  int fd_ {-1};

public:

  Camera() noexcept
  {
  }

  Camera(const std::string& device) : device_ {device}
  {
    this->open();
  }

  ~Camera() noexcept
  {
    try
      {
        this->close();
      }
    catch (const std::exception& e)
      {
        // Cannot throw from a destructor...
        std::cerr << e.what() << std::endl;
      }
  }

  Camera(const Camera&) = delete;  // not copy-constructible

  Camera(Camera&& other) : Camera {}
  {
    swap(*this, other);
  }

  Camera& operator=(const Camera&) = delete;  // not copy-assignable

  Camera&
  operator=(Camera&& other) noexcept
  {
    Camera tmp {};
    swap(*this, tmp);
    swap(*this, other);
    return *this;
  }

  friend void
  swap(Camera& first, Camera& second) noexcept
  {
    using std::swap;
    swap(first.device_, second.device_);
    swap(first.fd_, second.fd_);
  }

  void
  reopen()
  {
    this->close();
    this->open();
  }

  void
  open(const std::string& device = "")
  {
    if (this->fd_ >= 0)
      throw std::runtime_error {"camera already open"};
    if (!device.empty())
      this->device_ = device;
    if (this->device_.empty())
      throw std::runtime_error {"no associated device"};
    this->fd_ = camera_open(this->device_.c_str());
    if (this->fd_ < 0)
      throw std::runtime_error {"cannot open camera"};
  }

  void
  close()
  {
    if (this->fd_ >= 0)
      {
        if (camera_close(this->fd_) != 0)
          throw std::runtime_error {"cannot close camera"};
        this->fd_ = -1;
      }
  }
};

But are you sure that this is really a good design decision in the first place? Maybe the camera can just “reload” itself when necessary and not bother the user at all with this implementation detail?

Community
  • 1
  • 1
5gon12eder
  • 24,280
  • 5
  • 45
  • 92
  • 2
    I think it depens on the needed internal functionality of the class, whether he is able to just assign a new object. If the camera needs to clean up and free resources upon deletion, he needed to call `camera.cleanup()`, or use pointers and call `delete camera` so that the destructor cleans up. Anyway, as you mentioned, `reload()` would be much better. – Tobias Knauss Feb 27 '15 at 17:37
  • 3
    @TobiasKnauss I disagree with this. It is the responsibility of the type itself to free any resources it owns when assigned a new value. That's the whole point of RAII and much safer than having to remember to manually call a cleanup function on all paths of execution. Consider for example `std::string`. If you assign it a new value, it is the string's responsibility to free the old buffer if it is no longer needed. The added code demonstrates how this can be implemented. – 5gon12eder Feb 27 '15 at 17:51
  • OK, let me try to understand the alternatives. – João M. S. Silva Feb 27 '15 at 18:09
  • The first one is to use the advanced (for me) C++ features, kindly given in this answer. This is a possibility but somehow does not match the quality of the remaining code. In this excerpt, there is more C++ knowledge than in all my code ever. But I'll consider this if the other options are worse suited. Concerning the reload method: if I understand correctly, I must create a resource allocating function and a resource deallocating function, so that in case of a crash I can call deallocate and allocate. Is this correct? In this case the constructor does not allocate the resources, right? – João M. S. Silva Feb 27 '15 at 18:16
  • 1
    @JoãoM.S.Silva The idiomatic way in C++ if for the constructor to allocate resources and the destructor to de-allocate them. If you follow that idiom, then you'll see you don't need separate allocation/de-allocation functions. – juanchopanza Feb 27 '15 at 18:23
  • 1
    @JoãoM.S.Silva Well, you can define functions `Camera::load` and `Camera::unload`. Then the constructor calls `this->load()`, the destructor `this->unload()` and the `reload` function calls both of them. This is assuming that `unload()` is a non-op if there is nothing loaded. But even if the above example is too compilcated for now, you should at least `delete` the copy constructor and assignment operator as shown. Otherwise, you might get awkward bugs and risk double-destruction. – 5gon12eder Feb 27 '15 at 18:24
  • How would you rate these solutions: using the above C++ features and using reassign, creating the reload member function, or using pointers with `std::unique_ptr` like Neil Kirk suggested? – João M. S. Silva Feb 27 '15 at 18:33
  • I'm not sure whether I know enough about your situation to give definitive advice, but I don't see what the smart pointer buys you here. If you feel comfortable with adding a `reload` function, that seems a viable solution to me. – 5gon12eder Feb 27 '15 at 18:43
  • @5gon12eder: Please correct me if I'm wrong: In your answer you create a new camera. But where do you free the resources? Since the object is on the stack, the destructor is only called when the current function ends, which does not happen in your code, so the resource itself remains allocated, avoiding a successful creation of a new camera. Furthermore, the old object(s) stay(s) on the stack, causing an overflow if the creation of new objects happen too often. – Tobias Knauss Feb 27 '15 at 19:01
  • @TobiasKnauss I'm assuming that you can have more than one `Camera` object at a time. If this is not true, the above code would have to be modified. The destructor of the old `Camera` object is run as the assignment operator returns because I am swapping with a local `tmp` that goes out of scope. I have added a `close()` function to account for the possibility that you cannot have two cameras. Then you can do `cam.close(); cam = Camera {"/dev/cameras/front"};`. – 5gon12eder Feb 27 '15 at 19:07
  • Ok, updated the example even further to include all possibilities. It feels much like `std::fstream` now. – 5gon12eder Feb 27 '15 at 19:28