CODEWITHSUNDEEP
c++pointersmemory-managementmemory-leaks
Karlovsky120
Karlovsky120

Reputation: 6352

Dynamically allocated object producer

Look at this class:

//HPP
class A {
  public:
    A();
    ~A();
    B *fetchNew();

  private:
    B *currentValue;
}


//CPP
A::A() {}

A::~A {
   delete currentValue;
}

B *A::fetchNew() {
    delete currentValue;
    currentValue = new B();
    //apply magic to currentValue
    return currentValue;
}

This class holds a pointer to instance of class B. Every time fetchNew() is called, the old one gets deleted, a new one is allocated and magic is applied to it, after what new and shiny currentValue is returned.

This method is called very frequently (in real life, it's a method that returns a view matrix in a game's main loop, so it's called once every frame, about 60 times a second).

Once the A object gets deleted, it deletes the current currentValue, which would otherwise leak.

Is there a prettier way to achieve this?

EDIT:

Here's the actual code, since it has a twist (just the fetchNow() method):

glm::mat4x4 *Camera::getViewMatrix() {
    //transformation <<-apply shiny and fancy magic;

    delete viewMatrix;
    viewMatrix = new glm::mat4x4();
    *viewMatrix *= glm::mat4_cast(transformation->getRotation());
    *viewMatrix = glm::translate(*viewMatrix, transformation->getPosition());

    return viewMatrix;
}

Upvotes: 1

Views: 37

Answers (1)

πάντα ῥεῖ
πάντα ῥεῖ

Reputation: 1

Is there a prettier way to achieve this?

I would recommend rather to use a std::unique_ptr<B> than a raw pointer B*:

//HPP
#include <memory>
class A {
  public:
    A();
    ~A();
    std::unique_pty<B> fetchNew();

  private:
    // You don't need that: B *currentValue;
}

//CPP
A::A() {}

A::~A {
   // You don't need that: delete currentValue;
}

std::unique_ptr<B> A::fetchNew() {
    // You don't need that: delete currentValue;
    std::unique_ptr<B> newValue = std::make_unique<B>();
    // apply magic to newValue and dereference using * or -> as with any raw pointer
    return newValue;
}

This approach has several advantages:

  • You don't have to care about deletion or memory leaks in A
  • The transfer of ownership for the result of fetchNew() is semantically clear
  • It's a more clear API, the client will know they get ownership of the pointer and do not have to riddle if they need to delete that instance or not
  • You give the client the flexibility to determine the lifetime scope of the B instance themselves.

As for your edited addition it should look like:

std::unique_ptr<glm::mat4x4> Camera::getViewMatrix() {
    //transformation <<-apply shiny and fancy magic;

    std::unique_ptr<glm::mat4x4> viewMatrix = std::make_unique<glm::mat4x4>();
    *viewMatrix *= glm::mat4_cast(transformation->getRotation());
    *viewMatrix = glm::translate(*viewMatrix, transformation->getPosition());

    return viewMatrix;
}

Upvotes: 2

Related Questions