ProgramMemory: avoid duplicated lookups / removed at()
#7768
Conversation
|
"impossible" lookups with no exprid will be dealt with in a different PR. |
firewave
force-pushed
the
pm-at
branch
2 times, most recently
from
1cb862f to
7a04197
Compare
| if (value.tokvalue->exprId() > 0 && !pm->hasValue(value.tokvalue->exprId())) | ||
| if (value.tokvalue->exprId() == 0) | ||
| continue; |
There was a problem hiding this comment.
This should have been separate before since an at() call with no exprid would have failed.
|
It appears to not have much impact on performance but it gets rid of the |
|
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 missed that |
I consistently added a parameter to the getter (even though they might not be used - can be cleaned up later) and flipped one default. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
| 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; |
There was a problem hiding this comment.
I flipped the default behavior on getContainerEmptyValue() but addressed it via an explicit parameter in its only usage in vf_analyzers.cpp.
There was a problem hiding this comment.
Dont add the impossible parameters. They dont make sense at all.
There was a problem hiding this comment.
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.
| 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)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| return nullptr; | ||
| } | ||
|
|
||
| ValueFlow::Value* ProgramMemory::getValue(nonneg int exprid, bool impossible) |
There was a problem hiding this comment.
This should return a const ValueFlow::Value*. We dont ever mutate the ValueFlow::Value directly.
There was a problem hiding this comment.
We do. This was previously done from the non-const at().
| copyOnWrite(); | ||
|
|
||
| const auto it = find(exprid); | ||
| const bool found = it != mValues->cend() && (impossible || !it->second.isImpossible()); |
There was a problem hiding this comment.
The isImpossible can be checked by the consumers.
There was a problem hiding this comment.
I was thinking about doing that.
There was a problem hiding this comment.
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...
all
at()calls were proceeded byhasValue()so we can just directly fetch the value instead.