0
\$\begingroup\$

I am trying to implement smart pointers for our project. We're using FreeRTOS which is written with a C API so it was quite a challenge to implement sending smart pointers around but somehow I managed it by combining casts, new calls (dont really like that..) move semantics and some other tricks.

On the surface it behaves as expected, but I want to be sure there is not any memory leak. Furthermore, is there a way to use make_unique together with new? I have not really found a way yet.

Here is the code, it should work with an online C++ compiler (or use this link). It would be great if an expert could look over this. Maybe some things can be optimized, or maybe this code is not save at all and I just dont see it.

// Example program
#include <iostream>
#include <string>
#include <memory>
#include <cstring>

class DummyHelper {
public:
    DummyHelper(uint8_t someResource): someResource(someResource) {}

    virtual ~DummyHelper() {
       someResource = 0;
       std::cout << "Dtor called" << std::endl;
    }

    int getResource() {
       return someResource;
    }
private:
   int someResource = 0; 
};

class CommandMessage {
public:
    CommandMessage() = default;
    virtual~ CommandMessage() {};

    void setParameter(uint64_t parameter) {
        memcpy(rawContent.data() + 4, &parameter, sizeof(parameter));
    }

    uint64_t getParameter() const {
       uint64_t param;
       memcpy(&param, rawContent.data() + 4, sizeof(param));
       return param;
    }

private:
    std::array<uint8_t, 12> rawContent;
};

void setTestMessage(CommandMessage *testMsg,
       uint8_t someResource) {
  auto unique_pptr = new std::unique_ptr<DummyHelper>(new DummyHelper(someResource));
  uint64_t test_ptr = reinterpret_cast<uint64_t>(unique_pptr);
  testMsg->setParameter(test_ptr);
}

std::unique_ptr<DummyHelper> getUniquePtr(
       CommandMessage *testMsg) {
   uint64_t raw_ptr = testMsg->getParameter();
   auto unique_pptr = reinterpret_cast<std::unique_ptr<DummyHelper>*>(raw_ptr);
   auto unique_ptr = std::move(*unique_pptr);
   delete unique_pptr;
   return unique_ptr;
}


int main()
{
  auto storagePtr = std::make_unique<DummyHelper>(42);
  CommandMessage testMessage;
  std::cout << "Setting message" << std::endl;
  setTestMessage(&testMessage, 42);
  // a message has been set and can be sent, e.g. via a message queue

  // in another class, the pointer is retrieved.
  auto unique_ptr = getUniquePtr(&testMessage);
  DummyHelper & test = *unique_ptr;
  std::cout << test.getResource() << std::endl;
  unique_ptr.reset();
} 
\$\endgroup\$
  • \$\begingroup\$ Yes it leaks. SImply count the calls to new/delete and make sure they are the same. They are not. But I don't understand what you are trying to achieve. Why are you trying to create a smart pointer when you are using a smart pointer! You have a std::unique_ptr this is a smart pointer. \$\endgroup\$ – Martin York May 7 at 16:54
  • \$\begingroup\$ I think I am not able to send it via the message queue. If i create a simple unique_ptr and copy it into the message queue, the resource will be destroyed after going out of scope. I don't think it is possible to transfer ownership to the FreeRTOS message queue, given it was not programmed with C++ features in mind. I actually updated the code: I deleted the pointer in the getUniquePtr() call. \$\endgroup\$ – Spacefish May 7 at 18:22
  • \$\begingroup\$ You're making a pointer. To a unique_ptr. With new. I don't think you grasp the concept of smart pointers yet. \$\endgroup\$ – S.S. Anne May 7 at 23:17
1
\$\begingroup\$

Overview:

In relation to the message queue:

  • Since you are doing a manual new and delete that seems a bit of a waste.
  • Also the only thing you are creating is a unique_ptr.
  • The unique_ptr is never in stored correctly typed so it never does any work.

So I would not store a pointer to a std::unique_ptr I would just store the original pointer. I like the idea of passing in and retrieving a unique_ptr to the access function.


Why are you passing a pointer to CommandMessage? It can't be null (you never check so must be valid. To make this more formal pass by reference to indicate to the user that it should be a normal object.

void setTestMessage(CommandMessage *testMsg, uint8_t someResource)
std::unique_ptr<DummyHelper> getUniquePtr(CommandMessage *testMsg)

// I would do this:

void setTestMessage(CommandMessage& testMsg, uint8_t someResource)
std::unique_ptr<DummyHelper> getUniquePtr(CommandMessage& testMsg)

Note: Putting the '*' on the right by the variable is very "C" like. C++ considers this part of the type information so it is usually on the left with the type.


Why is the input message different from the output message?

void setTestMessage(CommandMessage *testMsg, uint8_t someResource)
std::unique_ptr<DummyHelper> getUniquePtr(CommandMessage *testMsg)

So input is a uint8_t but the output is std::unique_ptr<DummyHelper>? I would expect the interface to be symmetric.

template<typename T>
void sendMessage(CommandMessage& messageStream, std::unique_ptr<T>&& msg);

template<typename T>
std::unique_ptr<T> recvMessage(CommandMessage& messageStream);

template<typename T>
std::unique_ptr<T> recvMessage(CommandMessage& messageStream)
{
   // To store pointers in an integer type you should be using `std::intptr_t`
   // This is an int type that is guaranteed large enough to store a pointer.
   // Don't make the assumption that `uint64_t` is large enough.
   // It might be but that is not guaranteed for the future.
   std::intptr_t raw_ptr = messageStream.getParameter();

   return reinterpret_cast<T*>(raw_ptr);
}


template<typename T>
void sendMessage(CommandMessage& messageStream, std::unique_ptr<T>&& msg)
{
    messageStream.setParameter(msg.release());
}

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.