for applying std::forward don't you need a universal reference?

[juandent]

Hi,

I thought applying forward required that S was declared a universal reference like so:

you apply:

std::forward<S>(statement) 

shouldn't you have:

        template<typename S>
            prepared_statement_t<S> prepare_impl(S&& statement) {              // line 17524
// etc

instead of

        template<typename S>
            prepared_statement_t<S> prepare_impl(S statement) {              // line 17524
// etc

but you don't do it... is this a coding error? or am I mistaken?

Regards,
Juan

[fnc12]

Hi. This is by design.

[juandent]

On Scott Meyers excellent book Effective Modern C++, Item 25 states "Use std::move on rvalues references, std::forward on universal references" therefore not using universal references with forwarding does not move rvalues and is a mistake that can go undetected... let me come up with an example but please have this item in mind.

[fnc12]

There is no mistake. The code is correct. Any function can accept argument by value. This ability exists from the beginning of C++ and does not produce any errors. The only difference with accepting argument by rvalue reference is that argument will be copied if you pass it as is and it will be created from rvalue reference (moved) if you pass argument enclosed inside std::move call. Every prepare_impl has argument enclosed inside std::move and it doesn't produce any additional copies or undefined behaviors.
I used passing all arguments as rvalue references instead of values once C++11 is released but I faced compilation errors when you try to std::forward some tricky types. Sorry I don't remember what types they were (const char * probably). So my solution is that passing everything as rvalue reference is not a silver bullet and must be used only if this is exactly what you need. Scott Meyers is a very clever man and I respect his opinion. I also read several of his books about C++ tricks. But I never heard that Scott Meyers has written at least one working C++ application. He is a consultant. We can listen to his consultancy and decide whether to accept it or not.
Anyway thanks for pointing me to this. Actually sqlite_orm has a lot of legacy code which got to be refactored. I try to do it at my free time.

[fnc12]

Let me explain you the difference in details:

1. when you have a function like this

void prepare_impl(Foo &&foo) {
    //..
}

and call

Foo myFoo;
prepare_impl(std::move(myFoo));

then you pass initial myFoo object casted to rvalue reference right inside prepare_impl.

2. but when you have a function like this

void prepare_impl(Foo foo) {
    //..
}

and call

Foo myFoo;
prepare_impl(std::move(myFoo));

then you have a brand new object of type foo right inside prepare_impl with contents identical to initial myFoo. This content was moved using std::move call. std::move call doesn't move anything actually. It just casts lvalue reference to rvalue reference. Passing an rvalue reference to a function which accepts an object by value will try to create this object from passed rvalue reference that is call a constructor from rvalue reference. So zero extra copying will be called. Everything is ok. We just swapped inner content between two objects but we still good with the performance.

[juandent]

Please listen carefully. I have a code that displays the error. Basically using forward in a template parameter that is not a universal reference makes the forwarding incorrect (it always calls the rvalue version):

void makeLogEntry(std::string s) {}

struct Widget
{
	int m_number;
	Widget(int number) : m_number(number ) {}
};



void process( const Widget& lvalArg)
{
	std::cout << "Handling lvalue\n";
}

void process(Widget&& rvalArg)
{
	std::cout << "Handling rvalue\n";
}


template<typename T>
void LogAndProcessWithUR(T&& param)
{
	makeLogEntry("calling processs");
	process(std::forward<T>(param));
}

template<typename T>
void LogAndProcess(T param)		// no universal reference
{
	if (std::is_rvalue_reference_v<T>)
	{
		std::cout << "T is rvalue reference\n";
	}
	if (std::is_rvalue_reference_v<decltype(param)>)
	{
		std::cout << "param is rvalue reference\n";
	}

	makeLogEntry("calling processs");
	process(std::forward<T>(param));	// ALWAYS forwards to process(Widget&&) regardless of the object passed
}


void testForwarding()
{
	process(Widget{ 4 });	// call with rvalue
	Widget w{ 5 };
	process(w);

	LogAndProcess(Widget{ 4 });
	LogAndProcess(w);

	LogAndProcessWithUR(Widget{ 4 });
	LogAndProcessWithUR(w);
}

The result printed is:

Handling rvalue
Handling lvalue
Handling rvalue
Handling rvalue
Handling rvalue
Handling lvalue

See?

Regards,
Juan

[juandent]

Are you aware that rvalue reference is not universal reference although both look the same?

That is:

void process(Widget&& rvalArg)           // rvalue reference, should be moved
{
	std::cout << "Handling rvalue\n";
}
template<typename T>
void LogAndProcessWithUR(T&& param)     // universal reference: Not rvalue reference!!
{
	makeLogEntry("calling processs");
	process(std::forward<T>(param));
}

[fnc12]

Ok I see that LogAndProcess in your example calls rvalue version in both times. This code doesn't look like code with errors. This code is tricky but it is correct and it calls what you expect if you know C++ good enough.
In other words: what error do you see inside sqlite_orm related to this? What can go wrong?

[juandent]

param is always copied in LogAndProcess and therefore std::forward is equivalent to std::move.. we end up moving the copied object - but it is copied once! While LogAndProcessWithUR receives the lvalue or rvalue and forces no copy.
There is no actual error in doing it that way but it costs one additional unnecessary copy when going into LogAndProcess and the use of std::forward is misleading: it should be std::move because it unconditionally casts to rvalue reference

[fnc12]

Yes it is copied once in your case. But if you change LogAndProcess(w); to LogAndProcess(std::move(w)); it will never be copied.

[juandent]

I agree, but then we are making all objects rvalues even if they aren't! Isn't this a problem?

[fnc12]

Probably. Please show me the problem.

All I can see is that it can be a problem if you have two overloads like process in your example. I never saw cases where developers create function overloads with left/right value reference the only difference. As far as I know people avoid this. Scott Meyers doesn't avoid it cause his book is about tricky things. Yes it makes the book interesting. But what exactly can go wrong within sqlite_orm related to this issue right now?

Also Scott Meyers said that we need to use const_cast always inside operator[] overloading to avoid endless recursion. But the case is also very very seldom.

[juandent]

the only problem I see is that the lvalue is emptied upon std::move and should be not used anymore...

like so:

	process(Widget{ 4 });	// call with rvalue
	Widget w{ 5 };
	process(w);

	LogAndProcess(Widget{ 4 });
	LogAndProcess(std::move(w));            // w is erased

	LogAndProcessWithUR(Widget{ 4 });
	LogAndProcessWithUR(w);                // cannot use w here!

[fnc12]

why is it a problem? I mean when you have a code like this:

void bar(std::string argument) {
    //..
}
std::string myString = "some content"
bar(move(myString));

you also think that it has a problem?
Actually it has no problem cause myString can be used after bar call as empty string because object is empty after moving from it just like you have created it using a default constructor a line ago. Of course this behavior can be altered inside move constructor overloading but it is a bad practice and not expected by any developer.

[juandent]

I see now that you have an idiom in your code... if you always call std::move before sending the parameter to a template function then you are correct: this is no problem! But the only thing that I still feel is incorrect (call it misleading if you want) is the call to std::forward inside the template since it will always succeed in casting to rvalue (rather call std::move I think).
But there is no error, now I see. It is pass by value always together with moving the argument before the call..
the use of universal reference would discriminate between lvalues and rvalues, but I see that all you want is rvalues (since you call std::move) so I guess there is nothing lost. Save the use of std::forward, I think it should be std::move don't you agree?

[fnc12]

I don't want to eliminate the option of copying of object in case it is required. Accepting argument by value is a universal way of accepting argument cause it can be used for both copying (sometimes developers copy arguments by design) and moving.

[juandent]

yes but what about the misleading std::forward? it ALWAYS casts to rvalue when the template receives by value?? So it is not forwarding at all! It is "moving the value parameter"

[juandent]

this is no mistake - it just is a bit non-documenting the intention...

[fnc12]

I use std::forward as std::move with variadic arguments. I am wrong?

[juandent]

I think if std::forward Always moves then std::move should be used - the intention is clearer.
How is the case different for variadic arguments?

[juandent]

maybe I am being pedantic, if so I am sorry. I just like your library so much it deserves the best and most clear wording!

[fnc12]

How is the case different for variadic arguments?

It is the same. I mean that I use std::forward when I cannot use std::move because it can accept only one argument.

[fnc12]

maybe I am being pedantic, if so I am sorry. I just like your library so much it deserves the best and most clear wording!

Everything is ok. You are doing a great job. Thank you for taking care of it! I appreciate it!

[juandent]

You are very welcome! By the way I may be writing an article for a Costa Rican magazine related to using your amazing library and how it becomes so high-level and declarative by mixing C++ with SQL!! I will let you know if that project falls thru...

[juandent]

there is no wiki user guide only the text in your github site?

[juandent]

I started using triggers!!! Wow what a great addition!

[fnc12]

I started using triggers!!! Wow what a great addition!

Yeah. Thanks to @fmichaut-dev

[fnc12]

there is no wiki user guide only the text in your github site?

There is a short wiki on GitHub at Wiki tab also