ValueFlow: avoid various unnecessary copies #7583

firewave · 2025-06-10T10:09:59Z

No description provided.

firewave · 2025-06-10T10:10:39Z

Mostly taken from #6756 and discovered by reviewing the code. Still needs to be profiled.

firewave · 2025-06-10T11:21:44Z

Not much in terms of performance which is not surprising since these were found by review and not by profiling.

danmar · 2025-06-12T13:11:14Z

lib/valueflow.cpp

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


if more then one token has matching varid and this inner loop is executed twice then you will move values that are already moved.

Good catch. I was relying on the selfcheck/clang-tidy to catch these things.

Dropped . And I filed https://trac.cppcheck.net/ticket/13974 about detecting this.

…t()`

…r()`

…tion()`

pfultz2 · 2025-06-29T19:17:06Z

lib/valueflow.cpp

@@ -3155,10 +3155,10 @@ static void valueFlowLifetime(TokenList &tokenlist, ErrorLogger &errorLogger, co
        else if (tok->isUnaryOp("&")) {
            if (Token::simpleMatch(tok->astParent(), "*"))
                continue;
-            for (const ValueFlow::LifetimeToken& lt : ValueFlow::getLifetimeTokens(tok->astOperand1(), settings)) {
+            for (ValueFlow::LifetimeToken& lt : ValueFlow::getLifetimeTokens(tok->astOperand1(), settings)) {


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

pfultz2 · 2025-06-29T19:18:09Z

lib/valueflow.cpp

                if (!settings.certainty.isEnabled(Certainty::inconclusive) && lt.inconclusive)
                    continue;
-                ErrorPath errorPath = lt.errorPath;
+                ErrorPath& errorPath = lt.errorPath;


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

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.

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.

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.

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.

pfultz2 · 2025-06-29T19:20:26Z

lib/valueflow.cpp

@@ -4355,7 +4355,7 @@ static void valueFlowAfterAssign(TokenList &tokenlist,
                        continue;
                    ids.insert(value.tokvalue->exprId());
                }
-                for (ValueFlow::Value value : values) {
+                for (ValueFlow::Value& value : values) {


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

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.

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).

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).

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.

pfultz2 · 2025-06-29T19:27:57Z

lib/programmemory.cpp

@@ -327,11 +327,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;


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

firewave marked this pull request as draft June 10, 2025 10:10

firewave force-pushed the vf-move branch 3 times, most recently from 42d3603 to 762ebed Compare June 10, 2025 10:59

firewave marked this pull request as ready for review June 10, 2025 11:17

danmar reviewed Jun 12, 2025

View reviewed changes

firewave marked this pull request as draft June 12, 2025 13:19

firewave added 16 commits June 29, 2025 17:40

vf_settokenvalue.cpp: avoid unnecessary copies with `setTokenValueCas…

a5d5e0e

…t()`

valueflow.cpp: avoid unnecessary copies in valueFlowContainerSize()

750aa37

vf_settokenvalue.cpp: avoid unnecessary objects in setTokenValue()

321f5d1

valueflow.cpp: avoid unnecessary copies in valueFlowFunctionReturn()

a838908

valueflow.cpp: avoid unnecessary copy in valueFlowLifetime()

820df8c

programmemory.cpp: added comment

78aff9c

valueflow.cpp: avoid unnecessary copy in valueFlowSymbolicOperators()

2b51368

ValueFlow: avoid unnecessary copy in getLifetimeObjValue()

989e3bc

valueflow.cpp: avoid unnecessary copy in valueFlowAfterAssign()

7007d20

valueflow.cpp: avoid unnecessary copies in valueFlowInferCondition()

20319bb

valueflow.cpp: avoid unnecessary copy in valueFlowContainerSize()

58b9af5

valueflow.cpp: avoid unnecessary copy in valueFlowSafeFunctions()

503a76a

valueflow.cpp: avoid unnecessary copies with `valueFlowInjectParamete…

a1da6fa

…r()`

programmemory.cpp: avoid unnecessary copy in `programMemoryParseCondi…

bd75fca

…tion()`

vf_settokenvalue.cpp: void unnecessary copy in setTokenValue()

347823d

valueflow.cpp: avoid unnecessary copies in valueFlowFunctionReturn()

cb30d60

firewave force-pushed the vf-move branch from 762ebed to cb30d60 Compare June 29, 2025 15:58

firewave marked this pull request as ready for review June 29, 2025 16:03

pfultz2 reviewed Jun 29, 2025

View reviewed changes

firewave marked this pull request as draft July 3, 2025 09:24

ValueFlow: avoid various unnecessary copies #7583

Are you sure you want to change the base?

ValueFlow: avoid various unnecessary copies #7583

Uh oh!

Conversation

firewave commented Jun 10, 2025

Uh oh!

firewave commented Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

firewave commented Jun 10, 2025

Uh oh!

danmar Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

firewave commented Jun 10, 2025 •

edited

Loading

danmar Jun 12, 2025 •

edited

Loading