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

firewave

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

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

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

2. Behavior change in getContainerEmptyValue default — potential correctness issue