ProgramMemory: avoid duplicated lookups / removed `at()` by firewave · Pull Request #7768 · cppcheck-opensource/cppcheck

firewave · 2025-08-25T13:17:25Z

all at() calls were proceeded by hasValue() so we can just directly fetch the value instead.

firewave · 2025-08-25T13:25:35Z

"impossible" lookups with no exprid will be dealt with in a different PR.

firewave · 2025-08-25T13:39:53Z

-                if (value.tokvalue->exprId() > 0 && !pm->hasValue(value.tokvalue->exprId()))
+                if (value.tokvalue->exprId() == 0)
+                    continue;


This should have been separate before since an at() call with no exprid would have failed.

firewave · 2025-08-27T08:45:35Z

It appears to not have much impact on performance but it gets rid of the at() which has irked me for quite a while.

firewave · 2025-08-27T08:48:50Z

Should be done but I am seeing slightly less executions when profiling this, which should not be happening since it does not functionally change. So still needs looking into.

firewave · 2025-09-15T05:23:16Z

Should be done but I am seeing slightly less executions when profiling this, which should not be happening since it does not functionally change. So still needs looking into.

There is indeed a behavior change introduced by this - #7800 surfaced a difference the tests did not catch:

--- selfcheck.exp       2025-09-15 07:19:01.345202904 +0200
+++ selfcheck.res       2025-09-15 07:21:07.006911439 +0200
@@ -13626,6 +13626,8 @@
   tok2 possible symbolic=(tok->next)
 Line 2108
   true always 1
+Line 2109
+  . possible symbolic=(tok2)
 Line 2112
   & {lifetime[Address]=(temp),!0}
 Line 2113

firewave · 2025-09-15T05:35:52Z

There is indeed a behavior change introduced by this - #7800 surfaced a difference the tests did not catch:

I missed that getValue() does not return impossible values by default where hasValue() and at() do.

firewave · 2025-09-15T06:18:27Z

I missed that getValue() does not return impossible values by default where hasValue() and at() do.

I consistently added a parameter to the getter (even though they might not be used - can be cleaned up later) and flipped one default.

danmar · 2026-01-29T09:06:18Z

-                const ValueFlow::Value& v = utils::as_const(*pm).at(expr->exprId());
-                if (v.isIntValue())
-                    return v;
+            if (const ValueFlow::Value* v = utils::as_const(*pm).getValue(expr->exprId(), true)) {


if you think some cppcheck premium warnings are too noisy then please feel free to complain. I don't want that cppcheck premium is in the way for the team.

sonarqubecloud · 2025-09-15T06:29:05Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

firewave · 2025-09-15T08:19:45Z

-    bool getContainerSizeValue(nonneg int exprid, MathLib::bigint& result) const;
-    bool getContainerEmptyValue(nonneg int exprid, MathLib::bigint& result) const;
+    bool getContainerSizeValue(nonneg int exprid, MathLib::bigint& result, bool impossible = false) const;
+    bool getContainerEmptyValue(nonneg int exprid, MathLib::bigint& result, bool impossible = false) const;


I flipped the default behavior on getContainerEmptyValue() but addressed it via an explicit parameter in its only usage in vf_analyzers.cpp.

Dont add the impossible parameters. They dont make sense at all.

Read my comment. It was to make it clear what they are doing as they were inconsistent. But if we move the impossible logic out of them they will go away.

pfultz2 · 2025-09-16T19:45:45Z

            MathLib::bigint out = 0;
-            if (pm.getContainerEmptyValue(tok->exprId(), out))
+            // TODO: do we really went to return an impossible value?
+            if (pm.getContainerEmptyValue(tok->exprId(), out, true))


It is the correct behaviour, but the parameter and the comment makes it look like its wrong. The getContainerEmptyValue returns true or false if the container is empty or not. It will use impossible values internally to determine this as well(its really an implementation detail though).

This is why the impossible parameter should not be added.

It is inconsistent with the other getters. But if we move the impossible logic out of those of them as suggested, things will be clear.

As mentioned in another comment the impossible handling cannot easily be moved out of these functions.

pfultz2 · 2025-09-16T19:51:04Z

    return nullptr;
 }

+ValueFlow::Value* ProgramMemory::getValue(nonneg int exprid, bool impossible)


This should return a const ValueFlow::Value*. We dont ever mutate the ValueFlow::Value directly.

We do. This was previously done from the non-const at().

pfultz2 · 2025-09-16T19:53:54Z

+    copyOnWrite();
+
+    const auto it = find(exprid);
+    const bool found = it != mValues->cend() && (impossible || !it->second.isImpossible());


The isImpossible can be checked by the consumers.

I was thinking about doing that.

Ugh - it is already handled inside some of the getters. So it was even more inconsistent as it seemed to be. They also return the underlying MathLib::bigint so the impossible handling cannot just be externalized...

As the impossible handling cannot be easily moved out I just rebased it so it can be merged.

firewave · 2025-11-12T12:02:23Z

Should be done but I am seeing slightly less executions when profiling this, which should not be happening since it does not functionally change. So still needs looking into.

There is indeed a behavior change introduced by this - #7800 surfaced a difference the tests did not catch:
--- selfcheck.exp       2025-09-15 07:19:01.345202904 +0200
+++ selfcheck.res       2025-09-15 07:21:07.006911439 +0200
@@ -13626,6 +13626,8 @@
   tok2 possible symbolic=(tok->next)
 Line 2108
   true always 1
+Line 2109
+  . possible symbolic=(tok2)
 Line 2112
   & {lifetime[Address]=(temp),!0}
 Line 2113

I need to reproduce my mistake and add a test which causes this but that will probably take quite a while. Also making it more problematic is that I need to do it on an older commit because another change has also caused this change in the ValueFlow.

sonarqubecloud · 2025-11-12T12:04:52Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

danmar

I like that we replace hasValue=>at with a getValue. it looks good to me. I hope pfultz2 will review it since he is better on this.

…nctions

sonarqubecloud · 2026-02-18T10:53:44Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

danmar · 2026-04-26T09:48:54Z

I feel that my valueflow knowledge is shaky and asked AI to help me out with a review of this PR. Here is AI comments.. please feel free to ignore comments..

1. Performance regression in non-const `getValue()` (significant)

ValueFlow::Value* ProgramMemory::getValue(nonneg int exprid, bool impossible)
{
    if (mValues->empty())
        return nullptr;

    // TODO: avoid copy if no value is found
    copyOnWrite();
    ...
}

copyOnWrite() is called on every invocation even when the value doesn't exist and nullptr will be returned. The early-out only handles the empty-map case. Since the mutable getValue() is called in several hot paths (inc/dec ops, compound assignment, function-call argument scanning), this could trigger unnecessary COW copies on every pass. The TODO acknowledges this but ships the regression.

2. Behavior change in `getContainerEmptyValue` default — potential correctness issue

Old code hardcoded impossible=true internally:

// old
const ValueFlow::Value* value = getValue(exprid, true);

New default in the header is impossible=false:

bool getContainerEmptyValue(nonneg int exprid, MathLib::bigint& result, bool impossible = false) const;

The one visible call site in vf_analyzers.cpp compensates by explicitly passing true (with a // TODO: do we really want to return an impossible value? comment). However, the existing call at line 719 in vf_analyzers.cpp uses no argument — that call will silently change behavior once this PR merges. The TODO comment acknowledges uncertainty about correctness, which means this behavioral change needs a deliberate decision, not just a default parameter.

3. Typo in TODO comment

// TODO: do we really went to return an impossible value?

"went" should be "want".

4. Brace style inconsistency

Several new if blocks use a Stroustrup-style opening brace on a new line:

if (const ValueFlow::Value* v = utils::as_const(*pm).getValue(expr->exprId(), true))
{
    ...
}

The rest of the codebase uses same-line braces. This applies to at least 4 new blocks in programmemory.cpp.

5. `hasValue()` — new `impossible` parameter has no demonstrated use in this PR

hasValue() now accepts impossible = true as default and the parameter is wired into the return condition. But none of the call sites changed by this PR pass a non-default value to hasValue() — only getValue() is used with an explicit true at the replaced sites. The hasValue() calls at lines 443 and 459 in programmemory.cpp are untouched. The new parameter adds complexity without any demonstrated use in this change.

6. Missing test coverage for the new `impossible` parameter

The at() test was correctly removed, but no tests were added for:

The new non-const getValue() behavior
The impossible filtering via getIntValue, getTokValue, getContainerSizeValue, getContainerEmptyValue
The COW-triggering behavior of non-const getValue()

firewave · 2026-04-26T16:55:52Z

I am sorry but this really represents how clueless and useless AI actually is - beside it being a complete waste of unimaginable amounts of resources in any possible way. It is incredible how it spins two simple TODOs (which is what it all boils down to) into redundant, bloated drivel. I really baffles me how people think this helpful...

Except for the typo this is all intentional and mostly explained.

The changes have not really something to do with ValueFlow at all. It is about getting rid of duplicated lookups, the awkward at() (including exception handling) and aligning the bevahior of the accessors (for now - as noted in the comments it is mostly unnecessary but cannot easily be moved outside of the functions).

danmar · 2026-04-26T18:21:27Z

I am sorry but this really represents how clueless and useless AI actually is

well I am not against that any review comments from AI are ignored. I am thinking that this is a way to provide quicker reviews which have been asked for. My intention is that it's a complement not a replacement. I will continue to review manually also but it's slow and I can't dedicate more time.

Unfortunately ValueFlow is also an area where I feel I can't make really good reviews. I can miss really important problems when I review. So I don't feel comfortable about approving such PRs on my own..

I don't want to throw complete garbage at people. I looked at the AI comments before I posted and thought they were worth posting. It happens that I remove comments.

About (1) that did sound important to me but if you reject it I am sorry for the noise, personally I feel more confident after you explicitly rejected it. About (2) if there is behavior change that would have been a problem so I am glad you say this does not happen.
About (3) I assume there was a typo it was legit right?
About (4) is AI wrong so there is not inconsistent braces?
About (5) and (6) I just wanted to include these for consideration. I thought it would be possible you'd want to act.

firewave · 2026-04-26T22:00:25Z

(1) is just plain wrong. There is no regression. It matches the previous at() code
(2) there is no behavior change - there is reason it was added as an optional one
(3) is legit but negligible and if that is the only things AI actually turns up it need to go away immediately and forever
(4) it should not care about these things since there are formatters and linters in place - this is just more noise to the noise distracts from actually caring about the actual behavior of the code. Also just recently you weren't in favor of having consistent braces in a simplecpp PR.
(5) and (6) are intentional (as lined out in comments before) and are not tested because they might just be transitional (the idea was that they would not even make it into the actual code)

Of the few AI stuff I had to experience almost everything has been like this. It does not know what it is talking about, and distracts by being overly expeditionary and getting distracted by butterflies. It should be neither reviewing PRs not creating them.

It is like when I try to do PRs/tickets/reviews which in areas I am not very familiar or knowledgeable of (see performance related stuff in LLVM or internals in IWYU / most of the Cppcheck stuff is probably okay - albeit in parts only borderline). The only difference is that I am not trying to be smart about it and that I might actually learn that I should probably stay away from those things I do not understand and not waste anybodys time whereas AI will just keep spewing its uneducated filth.

What a waste this was ...

firewave force-pushed the pm-at branch from 44f69d1 to f6ed35e Compare August 25, 2025 13:22

firewave force-pushed the pm-at branch 2 times, most recently from 1cb862f to 7a04197 Compare August 25, 2025 13:32

firewave commented Aug 25, 2025

View reviewed changes

firewave force-pushed the pm-at branch from 7a04197 to f84c57f Compare August 30, 2025 11:14

firewave force-pushed the pm-at branch 2 times, most recently from ea4410b to 1aaf14f Compare September 15, 2025 06:15

firewave mentioned this pull request Sep 15, 2025

refs #12358 - CI-unixish.yml: added step which checks for AST and ValueFlow changes #7800

Open

firewave force-pushed the pm-at branch from 1aaf14f to 28064ac Compare September 15, 2025 06:25

firewave marked this pull request as ready for review September 15, 2025 06:25

github-advanced-security AI found potential problems Sep 15, 2025

View reviewed changes

firewave commented Sep 15, 2025

View reviewed changes

firewave mentioned this pull request Sep 16, 2025

Fix 13930: False positive: valueflow and other variable #7827

Merged

pfultz2 reviewed Sep 16, 2025

View reviewed changes

firewave marked this pull request as draft September 17, 2025 06:33

firewave force-pushed the pm-at branch from 28064ac to 31fa846 Compare November 12, 2025 11:59

firewave marked this pull request as ready for review November 12, 2025 11:59

firewave force-pushed the pm-at branch from 31fa846 to 9454a1b Compare January 26, 2026 13:34

danmar reviewed Jan 29, 2026

View reviewed changes

firewave added 2 commits February 18, 2026 11:46

ProgramMemory: consistently added impossible parameter to lookup fu…

3c1dd20

…nctions

ProgramMemory: avoid duplicated lookups / removed at()

1328408

firewave force-pushed the pm-at branch from 9454a1b to 1328408 Compare February 18, 2026 10:48

Conversation

firewave commented Aug 25, 2025

Uh oh!

firewave commented Aug 25, 2025

Uh oh!

firewave Aug 25, 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!

firewave commented Aug 27, 2025

Uh oh!

firewave commented Aug 27, 2025

Uh oh!

firewave commented Sep 15, 2025

Uh oh!

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

Uh oh!

Uh oh!

firewave commented Sep 15, 2025

Uh oh!

Check warning

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud Bot commented Sep 15, 2025

Quality Gate passed

Uh oh!

firewave Sep 15, 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!

firewave commented Nov 12, 2025

Uh oh!

sonarqubecloud Bot commented Nov 12, 2025

Quality Gate passed

Uh oh!

danmar left a comment

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud Bot commented Feb 18, 2026

Quality Gate passed

Uh oh!

danmar commented Apr 26, 2026

1. Performance regression in non-const getValue() (significant)

2. Behavior change in getContainerEmptyValue default — potential correctness issue

3. Typo in TODO comment

4. Brace style inconsistency

5. hasValue() — new impossible parameter has no demonstrated use in this PR

6. Missing test coverage for the new impossible parameter

Uh oh!

firewave commented Apr 26, 2026

Uh oh!

danmar commented Apr 26, 2026

Uh oh!

firewave commented Apr 26, 2026

Uh oh!

Reviewers

Assignees

Labels

firewave Aug 25, 2025 •

edited

Loading

firewave commented Sep 15, 2025 •

edited

Loading

firewave Sep 15, 2025 •

edited

Loading

1. Performance regression in non-const `getValue()` (significant)

2. Behavior change in `getContainerEmptyValue` default — potential correctness issue

5. `hasValue()` — new `impossible` parameter has no demonstrated use in this PR

6. Missing test coverage for the new `impossible` parameter