-
Notifications
You must be signed in to change notification settings - Fork 1.5k
ValueFlow: avoid unnecessary ValuePtr and Value copies
#4845
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
base: main
Are you sure you want to change the base?
Conversation
| {} | ||
|
|
||
| template<class U> | ||
| ValuePtr(U&& value) : mPtr(cloner<U>::move(&value)), mClone(&cloner<U>::apply) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
{}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fails to compile when we generate copies. I wonder if this even needs to be copyable at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current code crashes. Will have a proper look in the next few days.
No description provided.