Skip to content

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.

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 2 commits into
base: main
Choose a base branch
from
Draft

ValueFlow: avoid unnecessary ValuePtr and Value copies #4845

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d135695
ValueFlow: avoid unnecessary `ValuePtr` and `Value` copies [skip ci]
firewave Feb 28, 2023
8b49f67
infer [skip ci]
firewave Jan 26, 2025
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
12 changes: 6 additions & 6 deletions lib/forwardanalyzer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,8 @@
namespace {
struct ForwardTraversal {
enum class Progress : std::uint8_t { Continue, Break, Skip };
ForwardTraversal(const ValuePtr<Analyzer>& analyzer, const TokenList& tokenList, ErrorLogger& errorLogger, const Settings& settings)
: analyzer(analyzer), tokenList(tokenList), errorLogger(errorLogger), settings(settings)
ForwardTraversal(ValuePtr<Analyzer> analyzer, const TokenList& tokenList, ErrorLogger& errorLogger, const Settings& settings)
: analyzer(std::move(analyzer)), tokenList(tokenList), errorLogger(errorLogger), settings(settings)
{}
ValuePtr<Analyzer> analyzer;
const TokenList& tokenList;
Expand Down Expand Up @@ -900,24 +900,24 @@ namespace {
};
}

Analyzer::Result valueFlowGenericForward(Token* start, const Token* end, const ValuePtr<Analyzer>& a, const TokenList& tokenList, ErrorLogger& errorLogger, const Settings& settings)
Analyzer::Result valueFlowGenericForward(Token* start, const Token* end, ValuePtr<Analyzer> a, const TokenList& tokenList, ErrorLogger& errorLogger, const Settings& settings)
{
if (a->invalid())
return Analyzer::Result{Analyzer::Action::None, Analyzer::Terminate::Bail};
ForwardTraversal ft{a, tokenList, errorLogger, settings};
ForwardTraversal ft{std::move(a), tokenList, errorLogger, settings};
if (start)
ft.analyzer->updateState(start);
ft.updateRange(start, end);
return Analyzer::Result{ ft.actions, ft.terminate };
}

Analyzer::Result valueFlowGenericForward(Token* start, const ValuePtr<Analyzer>& a, const TokenList& tokenList, ErrorLogger& errorLogger, const Settings& settings)
Analyzer::Result valueFlowGenericForward(Token* start, ValuePtr<Analyzer> a, const TokenList& tokenList, ErrorLogger& errorLogger, const Settings& settings)
{
if (Settings::terminated())
throw TerminateException();
if (a->invalid())
return Analyzer::Result{Analyzer::Action::None, Analyzer::Terminate::Bail};
ForwardTraversal ft{a, tokenList, errorLogger, settings};
ForwardTraversal ft{std::move(a), tokenList, errorLogger, settings};
(void)ft.updateRecursive(start);
return Analyzer::Result{ ft.actions, ft.terminate };
}
7 changes: 3 additions & 4 deletions lib/forwardanalyzer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,20 +20,19 @@
#define forwardanalyzerH

#include "analyzer.h"
#include "valueptr.h"

class ErrorLogger;
class Settings;
class Token;
class TokenList;
template<class T> class ValuePtr;

Analyzer::Result valueFlowGenericForward(Token* start,
const Token* end,
const ValuePtr<Analyzer>& a,
const Token* end,ValuePtr<Analyzer> a,
const TokenList& tokenList,
ErrorLogger& errorLogger,
const Settings& settings);

Analyzer::Result valueFlowGenericForward(Token* start, const ValuePtr<Analyzer>& a, const TokenList& tokenList, ErrorLogger& errorLogger, const Settings& settings);
Analyzer::Result valueFlowGenericForward(Token* start, ValuePtr<Analyzer> a, const TokenList& tokenList, ErrorLogger& errorLogger, const Settings& settings);

#endif
20 changes: 10 additions & 10 deletions lib/reverseanalyzer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@

namespace {
struct ReverseTraversal {
ReverseTraversal(const ValuePtr<Analyzer>& analyzer, const TokenList& tokenlist, ErrorLogger& errorLogger, const Settings& settings)
: analyzer(analyzer), tokenlist(tokenlist), errorLogger(errorLogger), settings(settings)
ReverseTraversal(ValuePtr<Analyzer> analyzer, const TokenList& tokenlist, ErrorLogger& errorLogger, const Settings& settings)
: analyzer(std::move(analyzer)), tokenlist(tokenlist), errorLogger(errorLogger), settings(settings)
{}
ValuePtr<Analyzer> analyzer;
const TokenList& tokenlist;
Expand Down Expand Up @@ -251,7 +251,7 @@ namespace {
if (a) {
valueFlowGenericForward(nextAfterAstRightmostLeaf(assignTok->astOperand2()),
assignTok->astOperand2()->scope()->bodyEnd,
a,
std::move(a),
tokenlist,
errorLogger,
settings);
Expand All @@ -265,11 +265,11 @@ namespace {
if (a) {
valueFlowGenericForward(nextAfterAstRightmostLeaf(assignTok->astOperand2()),
assignTok->astOperand2()->scope()->bodyEnd,
a,
std::move(a),
tokenlist,
errorLogger,
settings);
valueFlowGenericReverse(assignTok->astOperand1()->previous(), end, a, tokenlist, errorLogger, settings);
valueFlowGenericReverse(assignTok->astOperand1()->previous(), end, std::move(a), tokenlist, errorLogger, settings);
}
}
}
Expand Down Expand Up @@ -302,7 +302,7 @@ namespace {
break;
if (condAction.isModified())
break;
valueFlowGenericForward(condTok, analyzer, tokenlist, errorLogger, settings);
valueFlowGenericForward(condTok, std::move(analyzer), tokenlist, errorLogger, settings);
}
Token* thenEnd;
const bool hasElse = Token::simpleMatch(tok->link()->tokAt(-2), "} else {");
Expand Down Expand Up @@ -331,7 +331,7 @@ namespace {
break;

if (!thenAction.isModified() && !elseAction.isModified())
valueFlowGenericForward(condTok, analyzer, tokenlist, errorLogger, settings);
valueFlowGenericForward(condTok, std::move(analyzer), tokenlist, errorLogger, settings);
else if (condAction.isRead())
break;
// If the condition modifies the variable then bail
Expand All @@ -350,7 +350,7 @@ namespace {
}
Token* condTok = getCondTokFromEnd(tok->link());
if (condTok) {
Analyzer::Result r = valueFlowGenericForward(condTok, analyzer, tokenlist, errorLogger, settings);
Analyzer::Result r = valueFlowGenericForward(condTok, std::move(analyzer), tokenlist, errorLogger, settings);
if (r.action.isModified())
break;
}
Expand Down Expand Up @@ -403,10 +403,10 @@ namespace {
};
}

void valueFlowGenericReverse(Token* start, const Token* end, const ValuePtr<Analyzer>& a, const TokenList& tokenlist, ErrorLogger& errorLogger, const Settings& settings)
void valueFlowGenericReverse(Token* start, const Token* end, ValuePtr<Analyzer> a, const TokenList& tokenlist, ErrorLogger& errorLogger, const Settings& settings)
{
if (a->invalid())
return;
ReverseTraversal rt{a, tokenlist, errorLogger, settings};
ReverseTraversal rt{std::move(a), tokenlist, errorLogger, settings};
rt.traverse(start, end);
}
6 changes: 3 additions & 3 deletions lib/reverseanalyzer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,14 @@
#ifndef reverseanalyzerH
#define reverseanalyzerH

#include "valueptr.h"

struct Analyzer;
class ErrorLogger;
class Settings;
class Token;
class TokenList;
template<class T>
class ValuePtr;

void valueFlowGenericReverse(Token* start, const Token* end, const ValuePtr<Analyzer>& a, const TokenList& tokenlist, ErrorLogger& errorLogger, const Settings& settings);
void valueFlowGenericReverse(Token* start, const Token* end, ValuePtr<Analyzer> a, const TokenList& tokenlist, ErrorLogger& errorLogger, const Settings& settings);

#endif
20 changes: 10 additions & 10 deletions lib/valueflow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3362,14 +3362,14 @@ static void valueFlowConditionExpressions(const TokenList& tokenlist,
condTok2,
makeConditionValue(1, condTok2, /*assume*/ true, !isBool, settings),
settings); // don't set '1' for non-boolean expressions
valueFlowGenericForward(startTok, startTok->link(), a1, tokenlist, errorLogger, settings);
valueFlowGenericForward(startTok, startTok->link(), std::move(a1), tokenlist, errorLogger, settings);
}

auto a2 = makeOppositeExpressionAnalyzer(true,
condTok2,
makeConditionValue(0, condTok2, true, false, settings),
settings);
valueFlowGenericForward(startTok, startTok->link(), a2, tokenlist, errorLogger, settings);
valueFlowGenericForward(startTok, startTok->link(), std::move(a2), tokenlist, errorLogger, settings);
}
}

Expand All @@ -3384,15 +3384,15 @@ static void valueFlowConditionExpressions(const TokenList& tokenlist,
auto a1 = makeSameExpressionAnalyzer(condTok2,
makeConditionValue(0, condTok2, false, false, settings),
settings);
valueFlowGenericForward(startTok, startTok->link(), a1, tokenlist, errorLogger, settings);
valueFlowGenericForward(startTok, startTok->link(), std::move(a1), tokenlist, errorLogger, settings);

if (is1) {
auto a2 =
makeOppositeExpressionAnalyzer(true,
condTok2,
makeConditionValue(isOp, condTok2, false, false, settings),
settings);
valueFlowGenericForward(startTok, startTok->link(), a2, tokenlist, errorLogger, settings);
valueFlowGenericForward(startTok, startTok->link(), std::move(a2), tokenlist, errorLogger, settings);
}
}
}
Expand All @@ -3410,7 +3410,7 @@ static void valueFlowConditionExpressions(const TokenList& tokenlist,
auto a1 = makeSameExpressionAnalyzer(condTok2,
makeConditionValue(0, condTok2, false, false, settings),
settings);
valueFlowGenericForward(startTok->link()->next(), scope2->bodyEnd, a1, tokenlist, errorLogger, settings);
valueFlowGenericForward(startTok->link()->next(), scope2->bodyEnd, std::move(a1), tokenlist, errorLogger, settings);

if (is1) {
auto a2 = makeOppositeExpressionAnalyzer(true,
Expand All @@ -3419,7 +3419,7 @@ static void valueFlowConditionExpressions(const TokenList& tokenlist,
settings);
valueFlowGenericForward(startTok->link()->next(),
scope2->bodyEnd,
a2,
std::move(a2),
tokenlist,
errorLogger,
settings);
Expand Down Expand Up @@ -3711,11 +3711,11 @@ static void valueFlowSymbolicInfer(const SymbolDatabase& symboldatabase, const S
std::vector<ValueFlow::Value> values;
{
SymbolicInferModel leftModel{tok->astOperand1()};
values = infer(leftModel, tok->str(), 0, tok->astOperand2()->values());
values = infer(std::move(leftModel), tok->str(), 0, tok->astOperand2()->values());
}
if (values.empty()) {
SymbolicInferModel rightModel{tok->astOperand2()};
values = infer(rightModel, tok->str(), tok->astOperand1()->values(), 0);
values = infer(std::move(rightModel), tok->str(), tok->astOperand1()->values(), 0);
}
for (ValueFlow::Value& value : values) {
setTokenValue(tok, std::move(value), settings);
Expand Down Expand Up @@ -5404,7 +5404,7 @@ static void valueFlowInjectParameter(const TokenList& tokenlist,
auto a = makeMultiValueFlowAnalyzer(arg, settings);
valueFlowGenericForward(const_cast<Token*>(functionScope->bodyStart),
functionScope->bodyEnd,
a,
std::move(a),
tokenlist,
errorLogger,
settings);
Expand Down Expand Up @@ -5932,7 +5932,7 @@ static void valueFlowUninit(TokenList& tokenlist, ErrorLogger& errorLogger, cons
}
auto partialReadsAnalyzer = std::make_shared<PartialReadContainer>();
auto analyzer = makeMemberExpressionAnalyzer(memVar.nameToken()->str(), tok, uninitValue, partialReadsAnalyzer, settings);
valueFlowGenericForward(start, tok->scope()->bodyEnd, analyzer, tokenlist, errorLogger, settings);
valueFlowGenericForward(start, tok->scope()->bodyEnd, std::move(analyzer), tokenlist, errorLogger, settings);

for (auto&& p : *partialReadsAnalyzer) {
Token* tok2 = p.first;
Expand Down
7 changes: 7 additions & 0 deletions lib/valueptr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ class CPPCHECKLIB ValuePtr {
static T* apply(const T* x) {
return new U(*static_cast<const U*>(x));
}
static T* move(T* x) {
return new U(std::move(*static_cast<const U*>(x)));
}
};

public:
Expand All @@ -47,6 +50,10 @@ class CPPCHECKLIB ValuePtr {
ValuePtr(const U& value) : mPtr(cloner<U>::apply(&value)), mClone(&cloner<U>::apply)
{}

template<class U>
ValuePtr(U&& value) : mPtr(cloner<U>::move(&value)), mClone(&cloner<U>::apply)
Copy link
Contributor

@pfultz2 pfultz2 Aug 30, 2023

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)
    {}

Copy link
Collaborator Author

@firewave firewave Aug 30, 2023

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.

Copy link
Collaborator Author

@firewave firewave Mar 14, 2024

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.

Copy link
Collaborator Author

@firewave firewave Jan 26, 2025

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.

Copy link
Collaborator Author

@firewave firewave Jan 26, 2025

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.

{}

ValuePtr(const ValuePtr& rhs) : mPtr(nullptr), mClone(rhs.mClone) {
if (rhs) {
mPtr.reset(mClone(rhs.get()));
Expand Down