Skip to content

ValueFlow: avoid various unnecessary copies #7583

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.

Sign up for GitHub

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

Draft
firewave wants to merge 11 commits into
base: main
Choose a base branch
from
+33 −40
Draft

ValueFlow: avoid various unnecessary copies #7583

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
9d8af20
valueflow.cpp: avoid unnecessary copies in `valueFlowContainerSize()`
firewave Aug 31, 2024
7e5de60
valueflow.cpp: avoid unnecessary copies in `valueFlowFunctionReturn()`
firewave Aug 31, 2024
249826b
valueflow.cpp: avoid unnecessary copy in `valueFlowLifetime()`
firewave Aug 31, 2024
d12fe81
valueflow.cpp: avoid unnecessary copy in `valueFlowAfterAssign()`
firewave Dec 25, 2024
ce371be
valueflow.cpp: avoid unnecessary copies in `valueFlowInferCondition()`
firewave Dec 25, 2024
b2427a6
valueflow.cpp: avoid unnecessary copy in `valueFlowContainerSize()`
firewave Dec 25, 2024
68d24d1
valueflow.cpp: avoid unnecessary copy in `valueFlowSafeFunctions()`
firewave Dec 25, 2024
ba28899
programmemory.cpp: avoid unnecessary copy in `programMemoryParseCondi…
firewave Aug 7, 2024
ade5c87
valueflow.cpp: avoid unnecessary copies in `valueFlowFunctionReturn()…
firewave Jun 10, 2025
7cd6bfb
vf_settokenvalue.cpp: avoid unnecessary copy in `setTokenValue()` [sk…
firewave Jan 9, 2025
8e0b759
valueflow.cpp: avoid unnecessary copies with `valueFlowForwardConst()…
firewave Dec 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions lib/programmemory.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,11 +364,13 @@ static void programMemoryParseCondition(ProgramMemory& pm, const Token* tok, con
if (endTok && findExpressionChanged(vartok, tok->next(), endTok, settings))
return;
const bool impossible = (tok->str() == "==" && !then) || (tok->str() == "!=" && then);
const ValueFlow::Value& v = then ? truevalue : falsevalue;
pm.setValue(vartok, impossible ? asImpossible(v) : v);
ValueFlow::Value& v = then ? truevalue : falsevalue;
Copy link
Contributor

@pfultz2 pfultz2 Jun 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be ValueFlow::Value&& v = then ? std::move(truevalue) : std::move(falsevalue).

const auto iv = v.intvalue;
// cppcheck-suppress accessMoved - FP #13628
pm.setValue(vartok, impossible ? asImpossible(std::move(v)) : v);
const Token* containerTok = settings.library.getContainerFromYield(vartok, Library::Container::Yield::SIZE);
if (containerTok)
pm.setContainerSizeValue(containerTok, v.intvalue, !impossible);
pm.setContainerSizeValue(containerTok, iv, !impossible);
} else if (Token::simpleMatch(tok, "!")) {
programMemoryParseCondition(pm, tok->astOperand1(), endTok, settings, !then);
} else if (then && Token::simpleMatch(tok, "&&")) {
Expand Down
60 changes: 25 additions & 35 deletions lib/valueflow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3006,10 +3006,10 @@ static void valueFlowLifetime(TokenList &tokenlist, ErrorLogger &errorLogger, co
if (tok2 && tok2 != op1 && (!tok2->variable() || !tok2->variable()->isArray()) && !(tok2->valueType() && tok2->valueType()->container))
continue;
}
for (const ValueFlow::LifetimeToken& lt : ValueFlow::getLifetimeTokens(tok->astOperand1(), settings)) {
for (ValueFlow::LifetimeToken& lt : ValueFlow::getLifetimeTokens(tok->astOperand1(), settings)) {
Copy link
Contributor

@pfultz2 pfultz2 Jun 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be written as for (ValueFlow::LifetimeToken&& lt : ValueFlow::getLifetimeTokens(tok->astOperand1(), settings)), although I think you might need to use move_iterators.

if (!settings.certainty.isEnabled(Certainty::inconclusive) && lt.inconclusive)
continue;
ErrorPath errorPath = lt.errorPath;
ErrorPath& errorPath = lt.errorPath;
Copy link
Contributor

@pfultz2 pfultz2 Jun 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be written as ErrorPath&& errorPath = std::move(lt.errorPath).

Copy link
Collaborator Author

@firewave firewave Jun 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would require two moves in the code so wouldn't that pessimize the code? Need to look into this. See also a somewhat related discussion I recently stumbled on: llvm/llvm-project#94798.

Copy link
Contributor

@pfultz2 pfultz2 Jun 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would require two moves in the code so wouldn't that pessimize the code?

Not its not. std::move is just a cast. A move will happen when its passed to a value parameter. Another std::move will still be needed since an rvalue reference variable will be an lvalue reference instead. So you will need to keep the std::move(errorPath) below.

See also a somewhat related discussion I recently stumbled on: llvm/llvm-project#94798.

Thats not related. That does call two move constructors.

Copy link
Collaborator Author

@firewave firewave Jun 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not its not. std::move is just a cast. A move will happen when its passed to a value parameter. Another std::move will still be needed since an rvalue reference variable will be an lvalue reference instead.

Thanks. As usual I have been stupid here. But the latter sentence answered a question while looking at the code.

What would be a case where a second std::move() would be unnecessary? I am wondering if the existing Clang/clang-tidy check would detect those.

Copy link
Contributor

@pfultz2 pfultz2 Jun 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be a case where a second std::move() would be unnecessary?

If you want to make a copy. But it seems like the check should require using std::move or std::as_const to make moves and copies explicit.

errorPath.emplace_back(tok, "Address of variable taken here.");

ValueFlow::Value value;
Expand Down Expand Up @@ -3795,16 +3795,15 @@ template<class ContainerOfValue>
static void valueFlowForwardConst(Token* start,
const Token* end,
const Variable* var,
const ContainerOfValue& values,
const Settings& settings,
int /*unused*/ = 0)
ContainerOfValue values,
const Settings& settings)
{
if (!precedes(start, end))
throw InternalError(var->nameToken(), "valueFlowForwardConst: start token does not precede the end token.");
for (Token* tok = start; tok != end; tok = tok->next()) {
if (tok->varId() == var->declarationId()) {
for (const ValueFlow::Value& value : values)
setTokenValue(tok, value, settings);
for (ValueFlow::Value& value : values)
setTokenValue(tok, std::move(value), settings);
} else {
[&] {
// Follow references
Expand All @@ -3813,7 +3812,7 @@ static void valueFlowForwardConst(Token* start,
return ref.token->varId() == var->declarationId();
});
if (it != refs.end()) {
for (ValueFlow::Value value : values) {
for (ValueFlow::Value& value : values) {
if (refs.size() > 1)
value.setInconclusive();
value.errorPath.insert(value.errorPath.end(), it->errors.cbegin(), it->errors.cend());
Expand All @@ -3829,7 +3828,7 @@ static void valueFlowForwardConst(Token* start,
continue;
if (v.tokvalue->varId() != var->declarationId())
continue;
for (ValueFlow::Value value : values) {
for (ValueFlow::Value& value : values) {
if (!v.isKnown() && value.isImpossible())
continue;
if (v.intvalue != 0) {
Expand All @@ -3849,15 +3848,6 @@ static void valueFlowForwardConst(Token* start,
}
}

static void valueFlowForwardConst(Token* start,
const Token* end,
const Variable* var,
const std::initializer_list<ValueFlow::Value>& values,
const Settings& settings)
{
valueFlowForwardConst(start, end, var, values, settings, 0);
}

static ValueFlow::Value::Bound findVarBound(const Variable* var,
const Token* start,
const Token* end,
Expand Down Expand Up @@ -3985,7 +3975,7 @@ static void valueFlowForwardAssign(Token* const tok,
});
std::list<ValueFlow::Value> constValues;
constValues.splice(constValues.end(), values, it, values.end());
valueFlowForwardConst(nextExpression, endOfVarScope, expr->variable(), constValues, settings);
valueFlowForwardConst(nextExpression, endOfVarScope, expr->variable(), std::move(constValues), settings);
}
if (isInitialVarAssign(expr)) {
// Check if variable is only incremented or decremented
Expand All @@ -4001,7 +3991,7 @@ static void valueFlowForwardAssign(Token* const tok,
value.bound = b;
value.invertRange();
value.setImpossible();
valueFlowForwardConst(nextExpression, endOfVarScope, expr->variable(), {std::move(value)}, settings);
valueFlowForwardConst(nextExpression, endOfVarScope, expr->variable(), std::list<ValueFlow::Value>{std::move(value)}, settings);
}
}
}
Expand Down Expand Up @@ -4209,7 +4199,7 @@ static void valueFlowAfterAssign(TokenList &tokenlist,
continue;
ids.insert(value.tokvalue->exprId());
}
for (ValueFlow::Value value : values) {
for (ValueFlow::Value& value : values) {
Copy link
Contributor

@pfultz2 pfultz2 Jun 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be for (ValueFlow::Value&& value : std::move(values)). This will make clear that the elements in the values vector has been moved.

Copy link
Contributor

@pfultz2 pfultz2 Jun 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we can't just move, we need to use move iterators. Something like this would do it:

template <class Container>
struct move_range_adaptor {
    move_range_adaptor(Container&& c) : container(std::forward<Container>(c)) {}
    
    auto begin() { return std::make_move_iterator(container.begin()); }
    auto end() { return std::make_move_iterator(container.end()); }

private:
    Container container;
};

template<class Container>
move_range_adaptor<Container> move_range(Container&& c) {
    return {std::forward<Container>(c)};
}

We just need to update our analysis to treat move_range as a move.

Copy link
Collaborator Author

@firewave firewave Jun 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will make clear that the elements in the values vector has been moved.

That is a good point. It could even be a readability check.

I have been avoiding using rvalues explicitly ever since they were causing copies instead of moves in a project I worked on in the past and we had to nuke them all over the place to get rid of performance issues (I no longer have access to the code base but I think there might be affected parts in a public repo and will try to reproduce that).

Copy link
Contributor

@pfultz2 pfultz2 Jun 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have been avoiding using rvalues explicitly ever since they were causing copies instead of moves in a project I worked on in the past and we had to nuke them all over the place to get rid of performance issues

I would assume this issue came from not moving rvalue references since rvalue reference variables become lvalues so they need to be moved again to be an rvalue.

Ideally there should be a check for this. Every implicit copy on an rvalue reference should be made as an explicit move or copy(althougth I am not sure how an explicit copy should be expressed).

Copy link
Contributor

@pfultz2 pfultz2 Jun 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every implicit copy on an rvalue reference should be made as an explicit move or copy(althougth I am not sure how an explicit copy should be expressed).

Actually, std::as_const could be used to explicity copy.

Copy link
Collaborator Author

@firewave firewave Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally there should be a check for this. Every implicit copy on an rvalue reference should be made as an explicit move or copy(althougth I am not sure how an explicit copy should be expressed).

It might also make sense to have a readability check for the usage of rvalues suggested by you. I will try to understand this properly, clean this up and also file tickets about this (with us and upstream).

if (!value.isSymbolicValue())
continue;
const Token* expr = value.tokvalue;
Expand Down Expand Up @@ -5052,7 +5042,7 @@ static void valueFlowInferCondition(TokenList& tokenlist, const Settings& settin
for (const ValuePtr<InferModel>& model : iteratorModels) {
std::vector<ValueFlow::Value> result =
infer(model, tok->str(), tok->astOperand1()->values(), tok->astOperand2()->values());
for (ValueFlow::Value value : result) {
for (ValueFlow::Value& value : result) {
value.valueType = ValueFlow::Value::ValueType::INT;
setTokenValue(tok, std::move(value), settings);
}
Expand Down Expand Up @@ -5815,10 +5805,10 @@ static void valueFlowFunctionReturn(TokenList& tokenlist, ErrorLogger& errorLogg

bool hasKnownValue = false;

for (const ValueFlow::Value& v : getCommonValuesFromTokens(returns)) {
setFunctionReturnValue(function, tok, v, settings, false);
for (ValueFlow::Value& v : getCommonValuesFromTokens(returns)) {
if (v.isKnown())
hasKnownValue = true;
setFunctionReturnValue(function, tok, std::move(v), settings, false);
}

if (hasKnownValue)
Expand All @@ -5844,10 +5834,10 @@ static void valueFlowFunctionReturn(TokenList& tokenlist, ErrorLogger& errorLogg
if (programMemory.empty() && !arguments.empty())
continue;
std::vector<ValueFlow::Value> values = execute(function->functionScope, programMemory, settings);
for (const ValueFlow::Value& v : values) {
for (ValueFlow::Value& v : values) {
if (v.isUninitValue())
continue;
setFunctionReturnValue(function, tok, v, settings);
setFunctionReturnValue(function, tok, std::move(v), settings);
}
}
}
Expand Down Expand Up @@ -6579,7 +6569,7 @@ static void valueFlowContainerSetTokValue(const TokenList& tokenlist, ErrorLogge
value.setKnown();
Token* start = initList->link() ? initList->link() : initList->next();
if (tok->variable() && tok->variable()->isConst()) {
valueFlowForwardConst(start, tok->variable()->scope()->bodyEnd, tok->variable(), {std::move(value)}, settings);
valueFlowForwardConst(start, tok->variable()->scope()->bodyEnd, tok->variable(), std::list<ValueFlow::Value>{std::move(value)}, settings);
} else {
valueFlowForward(start, tok, std::move(value), tokenlist, errorLogger, settings);
}
Expand Down Expand Up @@ -6663,8 +6653,8 @@ static void valueFlowContainerSize(const TokenList& tokenlist,
continue;
}

for (const ValueFlow::Value& value : values) {
valueFlowForward(nameToken->next(), var->nameToken(), value, tokenlist, errorLogger, settings);
for (ValueFlow::Value& value : values) {
valueFlowForward(nameToken->next(), var->nameToken(), std::move(value), tokenlist, errorLogger, settings);
}
}

Expand Down Expand Up @@ -6710,8 +6700,8 @@ static void valueFlowContainerSize(const TokenList& tokenlist,
const Token* constructorArgs = tok;
values = getContainerSizeFromConstructor(constructorArgs, tok->valueType(), settings, true);
}
for (const ValueFlow::Value& value : values)
setTokenValue(tok, value, settings);
for (ValueFlow::Value& value : values)
setTokenValue(tok, std::move(value), settings);
}
else if (Token::Match(tok->previous(), ",|(") && (Token::Match(tok, "{|%str%") || settings.library.detectContainer(tok))) {
if (Token* argTok = tok->previous()->astOperand2()) {
Expand Down Expand Up @@ -6748,8 +6738,8 @@ static void valueFlowContainerSize(const TokenList& tokenlist,
Token* rhs = tok->tokAt(2)->astOperand2();
std::vector<ValueFlow::Value> values = getInitListSize(rhs, containerTok->valueType(), settings);
valueFlowContainerSetTokValue(tokenlist, errorLogger, settings, containerTok, rhs);
for (const ValueFlow::Value& value : values)
valueFlowForward(containerTok->next(), containerTok, value, tokenlist, errorLogger, settings);
for (ValueFlow::Value& value : values)
valueFlowForward(containerTok->next(), containerTok, std::move(value), tokenlist, errorLogger, settings);
}
} else if (Token::Match(tok, ". %name% (") && tok->astOperand1() && tok->astOperand1()->valueType() &&
tok->astOperand1()->valueType()->container) {
Expand Down Expand Up @@ -7014,8 +7004,8 @@ static void valueFlowSafeFunctions(const TokenList& tokenlist, const SymbolDatab
argValues.back().valueType = ValueFlow::Value::ValueType::CONTAINER_SIZE;
argValues.back().errorPath.emplace_back(arg.nameToken(), "Assuming " + arg.name() + " size is 1000000");
argValues.back().safe = true;
for (const ValueFlow::Value &value : argValues)
valueFlowForward(const_cast<Token*>(functionScope->bodyStart), arg.nameToken(), value, tokenlist, errorLogger, settings);
for (ValueFlow::Value &value : argValues)
valueFlowForward(const_cast<Token*>(functionScope->bodyStart), arg.nameToken(), std::move(value), tokenlist, errorLogger, settings);
continue;
}

Expand Down
5 changes: 3 additions & 2 deletions lib/vf_settokenvalue.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,8 +258,9 @@ namespace ValueFlow
}

if (Token::simpleMatch(parent, "=") && astIsRHS(tok)) {
setTokenValue(parent, value, settings);
if (!value.isUninitValue())
const bool isUninit = value.isUninitValue();
setTokenValue(parent, std::move(value), settings);
if (!isUninit)
return;
}

Expand Down