Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added move semantics to AJAAncillaryList. #38

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

zerodefect
Copy link

The AJAAncillaryList encapsulates the following data types:

	AJAAncillaryDataList	m_ancList;		///< @brief	My packet list
	bool					m_rcvMultiRTP;	///< @brief	True: Rcv 1 RTP pkt per Anc pkt;  False: Rcv 1 RTP pkt for all Anc pkts
	bool					m_xmitMultiRTP;	///< @brief	True: Xmit 1 RTP pkt per Anc pkt;  False: Xmit 1 RTP pkt for all Anc pkts
	bool					m_ignoreCS;		///< @brief	True: ignore checksum errors;  False: don't ignore CS errors

...and AJAAncillaryDataList is a typedef for:

typedef std::vector <AJAAncillaryData*>			AJAAncillaryDataList;

which means the underlying types already have support for move semantics with a C++11 compiler.

I have the following function in my code:

AJAAncillaryList content_processor_aja_sdi_output::get_vanc_data_to_insert(...)
{
    AJAAncillaryList pkts;
    // ...
    return pkts;
}

Every time this function is called, instead of moving the packets, the packets get copied (allocation/deallocation per ancillary type) when they can merely be moved.

Changes tested within a linux environment - not tested on Windows. Need to be careful of introducing memory leaks, but I think I've covered all bases.

@zerodefect
Copy link
Author

Cleaned up the code/doxygen comments.

@zerodefect
Copy link
Author

There is a question on what should happen on a move. Read this here (for std::optional):

https://stackoverflow.com/questions/51805059/why-does-moving-stdoptional-not-reset-state

A commenter writes:

Unless otherwise specified, a moved-from object of class type is left in a valid but unspecified state. Not necessarily a "reset state", and definitely not "invalidated".

Why is that relevant. For the move-assignment operator, it could be argued that:

AJAAncillaryList & AJAAncillaryList::operator = (AJAAncillaryList && inRHS)
{
	if (this != &inRHS)
	{
		m_xmitMultiRTP = inRHS.m_xmitMultiRTP;
		inRHS.m_xmitMultiRTP = false;

		m_rcvMultiRTP = inRHS.m_rcvMultiRTP;			
		inRHS.m_rcvMultiRTP	= true;
		
		m_ignoreCS = inRHS.m_ignoreCS;
		inRHS.m_ignoreCS = false;
		
		Clear(); // Clear down any packets currently being held in 'this'
		m_ancList = std::move(inRHS.m_ancList);
	}
	return *this;
}

should become:

AJAAncillaryList & AJAAncillaryList::operator = (AJAAncillaryList && inRHS)
{
	if (this != &inRHS)
	{
		m_xmitMultiRTP = inRHS.m_xmitMultiRTP;
		m_rcvMultiRTP = inRHS.m_rcvMultiRTP;
		m_ignoreCS = inRHS.m_ignoreCS;
		
		Clear(); // Clear down any packets currently being held in 'this'
		m_ancList = std::move(inRHS.m_ancList);
	}
	return *this;
}

and:

AJAAncillaryList::AJAAncillaryList(AJAAncillaryList && inRHS)
	:	m_ancList(std::move(inRHS.m_ancList)),
		m_rcvMultiRTP(inRHS.m_rcvMultiRTP),
		m_xmitMultiRTP(inRHS.m_xmitMultiRTP),
		m_ignoreCS(inRHS.m_ignoreCS)
{
	// Reset RHS.
	inRHS.m_rcvMultiRTP = true;
	inRHS.m_xmitMultiRTP = false;
	inRHS.m_ignoreCS = false;
	// inRHS.m_ancList - already moved/reset.
}

should become:

AJAAncillaryList::AJAAncillaryList(AJAAncillaryList && inRHS) = default;

Thoughts? You are more than welcome to make changes to my commits as you so choose.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant