ValueFlow: avoid unnecessary ValuePtr and Value copies

[firewave]

No description provided.

[pfultz2]

We should only move when its passed as an rvalue references, not for lvalue references. Also, this might be called for the copy constructor and move constructor, so it needs to be constrained.

    template<class U, REQUIRES("Must be rvalue", !is_lvalue_reference<U>), REQUIRES("Must not be ValuePtr", !std::is_base_of<ValuePtr, U>)>
    ValuePtr(U&& value) : mPtr(cloner<U>::move(&value)), mClone(&cloner<U>::apply)
    {}

[firewave]

Thanks for the input. I need to check again what the actual impact of this was - probably not within the next few days.

I just pushed it again to clean up my local branches.

[firewave]

One case where I came across this was makeAnalyzer. You would expect that it is being moved on return but it is copied so we essentially create it twice every time - with the destruction on top.

[firewave]

This fails to compile when we generate copies. I wonder if this even needs to be copyable at all.

[firewave]

The current code crashes. Will have a proper look in the next few days.

[pfultz2]

This fails to compile when we generate copies.

Yes of course, because you are trying to pass a const pointer to a non-const pointer. When you use forwarding, you need to use std::forward to conditionally move, but you are trying to unconditionally move which wont work(although you are not really moving).

Its kind of complicated to pick apply if U&& is is a const ref. Instead, its simpler to just add a requires that its an rvalue or not an lvalue(like REQUIRES("Must be rvalue", !is_lvalue_reference<U>) where I showed in the snippet). This way it can use the const U& constructor for copies.

I wonder if this even needs to be copyable at all.

It does need to be copyable. The whole purpose of this class is so polymorphic type can be a regular type instead of a pointer.

[firewave]

It appears to affect the long-running tests TestIO::testScanfArgument and TestIO::testPrintfArgumentVariables.

[pfultz2]

This should exclude lvalue references and valueptr so the default copy/move constructor can be called:

Suggested change

	template<class U>
	template<class U, REQUIRES("Must be rvalue", !is_lvalue_reference<U>), REQUIRES("Must not be ValuePtr", !std::is_same<ValuePtr, typename std::decay<U>::type>)>

Normally, I do !is_base_of for forwarding constructors so its not picked over the default copy/move constructor(because forwarding reference overloads are always picked first) but in this case we are wanting a type with a base of ValuePtr so that would not work at all, so doing is_same with decay should hopefully work.

[firewave]

Thanks for the insights. Obviously I have no idea what I am doing.

I will give another look starting from scratch with your notes. So maybe that makes things clearer.

[firewave]

Please disregard the changes I pushed. I started with a better understanding but I think I still took a wrong turn somewhere. Will have another look tonight.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[firewave]

I started from scratch again and I am more confident now. The code makes sense and I looked at every copy (by adding an assert and running the unit tests) and there should no longer be any unnecessary ones. Still need to look at the case where it was taking up a visible part of the Ir.

Please have another look.

[firewave]

Still need to look at the case where it was taking up a visible part of the Ir.

Unfortunately that didn't help with this at all and the overall improvement from the changes here is minimal (about ~0,5% if I see it correctly).

It appears to affect the long-running tests TestIO::testScanfArgument and TestIO::testPrintfArgumentVariables.

The "problem" there is that a lot of ValuePtr objects are created for each ValueFlow:.setValues() call. The construction/destruction of the underlying shared pointers amounts to over 6% of the total Ir (that is almost 20% of the ValueFlow:.setValues() call) in those tests,