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

Open
firewave wants to merge 1 commit into
base: main
Choose a base branch
from
Open

ValueFlow: avoid unnecessary ValuePtr and Value copies #4845

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 7 additions & 7 deletions lib/forwardanalyzer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,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 @@ -316,7 +316,7 @@ namespace {
}

ForwardTraversal fork(bool analyze = false) const {
ForwardTraversal ft = *this;
ForwardTraversal ft = *this; // copy
if (analyze) {
ft.analyzeOnly = true;
ft.analyzeTerminate = true;
Expand Down Expand Up @@ -937,24 +937,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 };
}
4 changes: 2 additions & 2 deletions lib/forwardanalyzer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,11 @@ template<class T> class ValuePtr;

Analyzer::Result valueFlowGenericForward(Token* start,
const Token* end,
const ValuePtr<Analyzer>& a,
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 @@ -254,7 +254,7 @@ namespace {
if (a) {
valueFlowGenericForward(nextAfterAstRightmostLeaf(assignTok->astOperand2()),
assignTok->astOperand2()->scope()->bodyEnd,
a,
std::move(a),
tokenlist,
errorLogger,
settings);
Expand All @@ -268,11 +268,11 @@ namespace {
if (a) {
valueFlowGenericForward(nextAfterAstRightmostLeaf(assignTok->astOperand2()),
assignTok->astOperand2()->scope()->bodyEnd,
a,
a, // copy
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 @@ -305,7 +305,7 @@ namespace {
if (inLoop) {
if (Token::findmatch(tok->link(), "goto|break", tok))
break;
valueFlowGenericForward(condTok, analyzer, tokenlist, errorLogger, settings);
valueFlowGenericForward(condTok, analyzer, tokenlist, errorLogger, settings); // copy of analyzer
}
Token* thenEnd;
const bool hasElse = Token::simpleMatch(tok->link()->tokAt(-2), "} else {");
Expand Down Expand Up @@ -334,7 +334,7 @@ namespace {
break;

if (!thenAction.isModified() && !elseAction.isModified())
valueFlowGenericForward(condTok, analyzer, tokenlist, errorLogger, settings);
valueFlowGenericForward(condTok, analyzer, tokenlist, errorLogger, settings); // copy of analyzer
else if (condAction.isRead())
break;
// If the condition modifies the variable then bail
Expand All @@ -353,7 +353,7 @@ namespace {
}
Token* condTok = getCondTokFromEnd(tok->link());
if (condTok) {
Analyzer::Result r = valueFlowGenericForward(condTok, analyzer, tokenlist, errorLogger, settings);
Analyzer::Result r = valueFlowGenericForward(condTok, analyzer, tokenlist, errorLogger, settings); // copy of analyzer
if (r.action.isModified())
break;
}
Expand Down Expand Up @@ -406,10 +406,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);
}
2 changes: 1 addition & 1 deletion lib/reverseanalyzer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,6 @@ 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
16 changes: 8 additions & 8 deletions lib/valueflow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3432,14 +3432,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 @@ -3454,15 +3454,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 @@ -3480,7 +3480,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 @@ -3489,7 +3489,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 @@ -5476,7 +5476,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 @@ -6026,7 +6026,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
15 changes: 11 additions & 4 deletions lib/valueptr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,15 @@
#include <memory>

template<class T>
class CPPCHECKLIB ValuePtr {
class ValuePtr {
template<class U>
struct cloner {
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<U*>(x)));
}
};

public:
Expand All @@ -42,12 +45,16 @@ class CPPCHECKLIB ValuePtr {

ValuePtr() : mPtr(nullptr), mClone() {}

template<class U>
template<class U, REQUIRES("Must not be ValuePtr", !std::is_base_of<ValuePtr, U>)>
// cppcheck-suppress noExplicitConstructor
// NOLINTNEXTLINE(google-explicit-constructor)
ValuePtr(const U& value) : mPtr(cloner<U>::apply(&value)), mClone(&cloner<U>::apply)
{}

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

ValuePtr(const ValuePtr& rhs) : mPtr(nullptr), mClone(rhs.mClone) {
if (rhs) {
mPtr.reset(mClone(rhs.get()));
Expand All @@ -62,10 +69,10 @@ class CPPCHECKLIB ValuePtr {
return mPtr.get();
}

T& operator*() {
T& operator*() noexcept {
return *get();
}
const T& operator*() const {
const T& operator*() const noexcept {
return *get();
}

Expand Down
Loading