Scoping and variable lifetime.

I am currently writing a client interface for my smarthouse framework, trying to wrap my head around the quirks of c++'s memory handling.

In a static context given the function

void SmartHouse::processEvents(){

   ...

   boolean validPacket;
   TCP_InPacket packet = TCP_InPacket::parsePacket(&_wifiClient, validPacket);

   if(!validPacket){
       return;
   }else{
       //event callbacks
       //...
       //cleanup packet if created by new
   }
}
class TCP_InPacket : public TCP_Packet
{
public:

	//Factory 
	static TCP_InPacket parsePacket(Client* inputStream, boolean &sucess) {

		boolean requestOrResponse = readBoolean(inputStream);
		int32_t actionId = readInt(inputStream);
		int32_t uniqueRequestId = readInt(inputStream);
		//We don't need to use unsigned ints here as a payload might use java 
		//arrays which are max length of singed integers
		int32_t payloadLength = readInt(inputStream);

		//Now read the payload into an array
		byte* payloadData = new byte[payloadLength];
                //readBytes is a timmedCall therefor no additional handling is required unless timeout
		int32_t payloadRead = inputStream->readBytes(payloadData, payloadLength);

		if (payloadRead != payloadLength) {
			//We couldn't read enough bytes without timing out.
			//The packet is lost.
			sucess = false;
			Serial.print("Timeout packet lost");
			delete[](payloadData);
			return;
		}

		//Construct tcp packet
		sucess = true;
		return TCP_InPacket(requestOrResponse, actionId, uniqueRequestId, payloadLength, payloadData, inputStream);
	}


	TCP_InPacket(boolean response, int32_t actionId, int32_t uniqueRequestId, int32_t payloadLength, byte* payload, Client* client) : TCP_Packet(response, actionId, uniqueRequestId, client) {
		_payloadLength = payloadLength;
		_payload = payload;
	};
}

Is this valid code or am I correct to assume that this will eventually lead to unexpected behavior as the TCP_InPacket created in the return line will cease to exist as soon as the return executes? Do I have to use the new keyword and let the function return a pointer to the object instead?

Does the same apply for structures? Do I need to use return new CRGB?

CRGB TCP_InPacket::readColor() {
	//Colors are send over the network by it's 32 bit int representation.
	int32_t colorAsInt = readInt();
	return CRGB(colorAsInt);
}

Declaring you function as static means you can call it like this:

TCP_InPacket::parsePacket(...);

If you remove the static keyword then the only way you can call it is:

TCP_InPacket packet;
packet.parsePacket(...);

Creating commonly used functions as static members of a class just creates neat containment should you have other functions that do something slightly different but that you want them to have the same function name.

If you declared them as standard C functions with the same names then you would run into compiler errors.

Thank you. I am aware of how static functions work and don't get a compiler error. The only issue I am facing is if the code will eventually fail because I am not sure if the object instance created in the return line

return TCPInPacket() gets created on the heap or it's lifetime is over as soon as the stack pops.

It's more about usage of the new keyword than anything else.

Edit: After doing some more research on stack overflow I saw that it's perfectly valid code even without the pointer. I got confused since objects created in functions fall out of memory, but if you return them the copy constructor is called therefore it lives even outside the function. Coming from a managed language like java you have to question every little thing when it comes to memory management.

What will happen here is that each time parsePacket is executed, it will allocate more bytes off the heap for payloadData. These bytes are freed if the read failed, but otherwise not.

If the function that invoked parsePacket does not delete this memory (which it can't, because it's buried insde a TCP_inpacket structure) , then the heap will quickly be consumed, new byte[payloadLength] will begin returning NULL, and badness will ensue as the code treats that NULL as if it were a real address it could fool about with.

INstead of allocating payloadData dynamically, create a global or static chunk of memory to accomodate it, and check that payloadLength does not exceed he size of it. After all, memory on the arduino is limited, and as a practical matter you cannot accommodate arbitrarily large payloads anyway.

Alternatively, supply TCP_inpacket with a destructor whose job it is to delete its buffer, by moving payloadData to a class variable. But this is a bit heavyweight for the arduino, which only has one thread going at a time. I'm not 100% sure about destructor semantics.

PaulMurrayCbr:
Alternatively, supply TCP_inpacket with a destructor whose job it is to delete its buffer, by moving payloadData to a class variable. But this is a bit heavyweight for the arduino, which only has one thread going at a time. I'm not 100% sure about destructor semantics.

Thanks for noticing, I omitted the destructor to keep a "smallish" example

/~TCP_InPacket() {
		delete[] _payload;
	}

Everything running on a nodemcu so gives me a bit more head room to play around with. these packets are rather short lived, getting parsed handled in an event callback and deleted immediately. I might want to play around a little with smart pointers to hide everything from the programmer.

If I could address this part of your question:

if the code will eventually fail because I am not sure if the object instance created in the return line

return TCPInPacket() gets created on the heap or it's lifetime is over as soon as the stack pops.

There are a variety of combinations for passing and returning non-simple types (something besides a value or a pointer). In the code you've shown us, a method that "returns" an instance is magically morphed into a method that has an extra argument for the caller's instance to be passed in. Depending on the prototype and assignment operators or copy constructors for that class, the method operates on that "secret" argument. It doesn't really "return" the local variable, because the "local" variable is really passed in by the caller as an argument.

As long as the return type is not a pointer or reference to the local instance, you'll be fine. That is, DON'T do this:

TCP_InPacket * parsePacket( ... )
{
   TCP_InPacket retPkt( ... );
     ...
   return &retPkt;   //  BAD!  Caller will get a pointer to a destructed packet
}

:stuck_out_tongue: