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

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

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.

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

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.

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.

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

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.

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

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

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

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

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.

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