Returning pointer to an object from a method

Hello C++ experts, I am having a problem with system (microcontroller) stopping after some consecutive method calls, and I am pretty sure it is because of my incorrect pointer handling and corrupting heap memory.

So here is a method that I am calling

Code:

---


Message* MessageProcessor::processReceivedData(void)
{
    uint8_t receivedByte;
   
    if ( this->serialPort->available() )
    {
      this->serialPort->readBytes(&receivedByte, 1);
      this->messageDeserialization->deserialize(receivedByte);
      if ( this->messageDeserialization->isDeserialized() == MESSAGE_DESERIALIZATION_OK )
      {
        int8_t receieveBufferLen;
        uint8_t* receieveBuffer;
        int8_t retVal;
           
        receieveBufferLen = this->messageDeserialization->getReceiveBufferLen();
        receieveBuffer = new uint8_t[receieveBufferLen];
        retVal = this->messageDeserialization->getDeserializedBuffer(receieveBuffer, receieveBufferLen);
        Message message = this->messageDecoder->decode( receieveBuffer, receieveBufferLen );
        return &message;
      }
    }
    return NULL;
}

---

And from main.cpp I am calling this function and after call I delete pointer

Code:

---


int main(void)
{
    .
    .
    .
   
    while (true)
    {
        .
        .
        .
       
        Message* message = NULL;
        message =  messageProcessor.processReceivedData();
        if ( message != NULL )
        {
            if ( message->getDataType() == MESSAGE_DATA_TYPE_KEEP_ALIVE )
            {
                serialPortPc.writeString("I AM ALIVE\n\r");
            }
            serialPortPc.writeString("OK\n\r");
        }
        else
        {
            serialPortPc.writeString("NOK\n\r");
        }
        delete message;
       
        .
        .
        .
    }
}

---

I assume that the problem is because of the following reason:
1. after processReceivedData returns, it returns COPY of a pointer
2. after that I try to delete COPY of pointer, which doesn't do anything effective (I would need to return double pointer to this be effective ?)

I would rather prefer returning copy of an object, but than I do not know how to indicate error by return value (in my case returning NULL pointer in the last line of processReceivedData method.

I feel that my approach is completly incorrect since I am even returning pointer from my method, and making user having obligation to deal with memory (by calling delete).

You are returning a pointer to a local variable, but local variables cease to exist after their function returns. You need to return dynamically-allocated memory or the actual struct data itself.

It may be best to have the decode function return dynamically-allocated memory. Alternatively, since you are returning the class itself instead of a pointer from decode, you could do the same from this function.

As for your suppositions, a COPY of a pointer is just as good as the original. They are identical.

As for returning the class/struct itself (instead of a pointer) and also being able to signal an error, you could return the data through the function parameters and the status through the return value.

Code:

---

bool MessageProcessor::processReceivedData(Message *mess) {
    if (...) {
        *mess = ...->decode();
        return true;
    }
    return false;
}

int main() {
    Message mess;
    if (messageProcessor.processReceivedData(&mess))
        ...
}

---

BTW, we don't use NULL in C++. Pre-C++11 we use 0. C++11 and beyond we use nullptr.

If I decode function returns dynamically allocated memory I still have to delete it after function call? What about returning references is that any safer, but then other question raises, since I cannot delete reference how am I supposed to free the memory?
Also when I before asking for help I had situation like this:

Code:

---

... processReceivedData(void)
{
  Message* messagePtr = ...->decode();
  Message message = *messagePtr;
  delete messagePtr;
  return &message;
}

---

is this valid? Deleting dereferncing pointer to assign it to an object, and after that deleting it. Will return value (&message) get corrupted in this case?

[/code]

Quote:

---

If I decode function returns dynamically allocated memory I still have to delete it after function call?

---

Yes!

Quote:

---

Deleting dereferncing pointer to assign it to an object, and after that deleting it. Will return value (&message) get corrupted in this case?

---

Yes, message gets created on line #4 and dies with the function call. You simply cannot refer to it that way. Copies or pointers (C++ has additional pointer types to play with) to heap-allocated data are the only way to return stuff.

Quote:

---

Originally Posted by Guest

Yes!


Yes, message gets created on line #4 and dies with the function call. You simply cannot refer to it that way. Copies or pointers (C++ has additional pointer types to play with) to heap-allocated data are the only way to return stuff.

---

Okay guys thank you for your suggestions, basically when I think about your suggestions, that is how I would handle it in C, giving pointer as parameter and returning a value.

In the method decode I allocate new message object, but I never delete it, my system works properly but I simply do not understand why is it working. Why no heap corruption has happened, since I never freed memory that I allocated in the decode method?

Code:

---

Message* MessageDecoder::decode(uint8_t* deserializedBuffer, uint8_t deserializedBufferLen)
{

  Message* message;
   
  .
  .
  .
 
  message = new Message( dataType, dataLength, data);
   
  .
  .
  .
 
  return message;   
}

---

Code:

---

int8_t MessageProcessor::processReceivedData(Message* message)
{
    .
    .
    .
    // Returning pointer to a Message object
    message = this->messageDecoder->decode( receieveBuffer, receieveBufferLen );
    return MESSAGE_PROCESSOR_OK;
    .
    .
    .
}

---

Code:

---

int main(void)
{
    Message message(0, 0, NULL);
    int8_t retVal;
    .
    .
    .
   
    while (true)
    {
        retVal = messageProcessor.processReceivedData(&message);
        if ( retVal == MESSAGE_PROCESSOR_OK )
        {
            pcSerialPort.writeString("JEDAN\n\r");   
        }
       
        .
        .
        .
    }
}

---

Quote:

---

In the method decode I allocate new message object, but I never delete it, my system works properly but I simply do not understand why is it working.

---

There's nothing inherently illegal in not freeing allocated memory. It just means the process will hold on to that memory until it terminates all together. Complex programs you use daily (like Web Browsers) probably have a few memory leaks here and there, but it doesn't have to result in any problems at runtime. The danger with regards to pointers is trying to accessing memory that has been freed!

Here and example using std::unique_ptr. My C++ is a bit rusty in that regard but I think it should be right:

Code:

---

#include <iostream>
#include <memory>

using namespace std;

struct Message {
    Message(int n) : member(n) {
        cout << "Message created." << endl;
    }
    ~Message() {
        cout << "Message destroyed." << endl;
    }
    int member;
};

unique_ptr<Message> get_message(int value) {
    auto message = make_unique<Message>(value); // std::make_unique heap-allocates a "Message" object for you, and you pass constructor arguments directly
    cout << "Message holds '" << message->member << "'" << endl; // Message is a std::unique_ptr now, and behaves mostly like a regular one, e.g. we can refer to its member
    return message; // We return the pointer by value (i.e. a copy)
}

int main()
{
    auto p = get_message(42);
} // p goes out of scope and automatically deallocates the memory it points to

/* Prints:
  Message created.
  Message holds '42'
  Message destroyed.
*/

---

As you can see, this allows you to allocate heap memory and be sure it does with the pointer pointing to it.

Offtopic: Reply pending approval (again). I wonder what I'm doing wrong to trigger this. I'm well aware of the SPAM issue on this board, but since this forum is pretty dead, adding another hurdle sounds like a bad idea. What if newcomers have to wait several hours until their question gets approved? I imagine they'll try someplace else.