Karlovsky120 Karlovsky120 - 28 days ago 7
C++ Question

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;
}

Answer

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;
}
Comments