Potential bug: dangerous code: Remove std::move from Service API v2 macros

[bennettpeter]

The Services API macros for generating the set methods use a std::move call to set values that are QString, QDateTime, QVariantMap, or QVariantList if the parameter is not const. This resulted in a bug where the logging parameters were cleared out by an API call to access backend information. see c92362e. If the parameter is defined as const, the object is copied.

There may be other cases where a value is inadvertently being lost due to the parameter not being defined as const.

Fix this by eliminating the std::move for QString and investigate whether it should be eliminated for the other types also.

Reference

mythtv/mythtv/libs/libmythbase/http/mythhttpservice.h

Line 81 in c92362e

set##Name(T& value) { m_##Name = std::move(value); } \

[kmdewaal]

This macro looks like it generates inline code instead of a function call.
If a function call is used then, if parameters are passed by value, a copy is made. So if you write a "set" function with a QString as parameter then the function gets a copy of the QString. Copying this into a m_whatever class variable would then involve an additional copy. Preventing this (useless) copy is the reason to use a std::move.
However, if you a macro then it looks like a function call but there is no function call made and actually variable names are substituted; there is NO copy made of the parameters.
If you then use a std::move then what looks as a function parameter is being destroyed and that is what happens here.
The conclusion is therefore that, because we use macro's as set functions we should NOT use std::move.

[bennettpeter]

As a test, I commented these lines that generate the std::move:

mythtv/mythtv/libs/libmythbase/http/mythhttpservice.h

Lines 76 to 81 in b9354c9

	template <class T> \
	typename std::enable_if<std::is_same<T, QString>::value || \
	std::is_same<T, QDateTime>::value || \
	std::is_same<T, QVariantMap>::value || \
	std::is_same<T, QVariantList>::value, void>::type \
	set##Name(T& value) { m_##Name = std::move(value); } \

I also reversed the commit c92362e
Everything builds correctly.
I tested the Myth/GetBackendInfo call and that works OK, the logPropagateArgs do not get cleared
I ran some large queries of GetProgramGuide several times. All works fine and there did not seem to be any memory leakage.

Any problems with removing those lines and reverting c92362e?

[kmdewaal]

I think that I know what is wrong in line 81 of mythhttpservice.h
If you pass an object by value and then assign it, e.g.:
setLogArg(QString logArg) { m_logArg = logArg; }
then calling setLogArg creates a copy of the logArg and then the assignment to m_logArg creates a new copy; the first copy is later deleted automatically. There is never a memory leak but there is a temporary object created and removed.

There are two ways of copying a value without creating a temporary object that is later deleted.

The first is to to define the setLogArg with a reference to a QString.
The assignment to m_logArg then creates the object. No temporary object is made.

The second is to define the setLogArg with a QString parameter, and then using std::move in the assignment to m_logArg. The temporary object is then moved to m_logArg. This is just as effient as the previous method.

What goes wrong here in line 81 is that it has a reference to the QString as parameter and then it does the std::move to copy that reference to m_logArg. This then destroys the original QString.
So you cannot do it both, you can either pass a reference and then create an object when copying or you can create an object to pass by value and then std::move that object.

So instead of this:
set##Name(T& value) { m_##Name = std::move(value); }
you can do either this:
set##Name(T value) { m_##Name = std::move(value); }
or this
set##Name(T& value) { m_##Name = value; }

[kmdewaal]

But, to answer your question

Any problems with removing those lines and reverting c92362e?

No problem, just go ahead.
This means you chose for the set function that takes the reference and then makes a copy in the assignment. OK.

[linuxdude42]

I'm ok with this fix.