fixed #13513 - adjusted access of known int values by firewave · Pull Request #7181 · cppcheck-opensource/cppcheck

firewave · 2025-01-05T01:16:02Z

No description provided.

danmar

I like it.. good catch

firewave · 2025-01-18T16:34:04Z

-            if (dimension_.tok && (dimension_.tok->hasKnownIntValue() ||
-                                   (dimension_.tok->isTemplateArg() && !dimension_.tok->values().empty()))) {
-                dimension_.num = dimension_.tok->getKnownIntValue();
+            if (dimension_.tok) {
+                if (dimension_.tok->hasKnownIntValue())
+                    dimension_.num = dimension_.tok->getKnownIntValue();
+                else if (dimension_.tok->isTemplateArg() && !dimension_.tok->values().empty()) {
+                    assert(dimension_.tok->values().front().isKnown());
+                    dimension_.num = dimension_.tok->values().front().intvalue;
+                }


@chrchr-github Please have a look. You changed this logic a few days ago in 46781df.

For template arguments, the values are only possible, not known. But since getKnownIntValue() just returns the first one, it is what we want in this case (I think).

So like the previous behavior? Just unconditionally use the first value and set dimension_.known to true?

Yes, pretty much. I imagine/hope that template arguments can have at most one value.

Maybe we should set those values to known in the future, and prevent unwanted knownConditionTrueFalse warnings in a different way.

I imagine/hope that template arguments can have at most one value.

I will just add a different assert.

pfultz2 · 2025-01-19T10:23:22Z

            continue;

-        const MathLib::bigint rhsvalue = tok->astOperand2()->values().front().intvalue;
+        const MathLib::bigint rhsvalue = tok->astOperand2()->getKnownValue()->intvalue;


Why not getKbownIntValue?

Because the preceding check is hasKnownValue().

That is a bug, it should be checking for a known int value since it's creating an int value below.

I see.

But the value is created on tok which has another hasKnownValue() check above. So that does not seem related (to me). Unless all of them need to be adjusted.

Also given the information in the other comment if this already has a non-int value and we set the int-value wouldn't that break that the case that it may only hold a single value (if that is even possible)?

It is related, all the checks in this pass should check for int value only.

pfultz2 · 2025-01-19T10:28:57Z

+        return nullptr;
+    auto it = std::find_if(mImpl->mValues->begin(), mImpl->mValues->end(), std::mem_fn(&ValueFlow::Value::isKnown));
+    return it == mImpl->mValues->end() ? nullptr : &*it;
+}


There can be multiple known values of different types on a token and with the exception of int, they can be in any order. It seems like it would always be better to use the overload that takes the type.

There can be multiple known values of different types on a token and with the exception of int, they can be in any order.

Good to know. So I guess there is no need for an intermediate step and this can simply be cleaned up.

Could that assumption be used to simplify the lookup? A getKnownIntValue() which will bail out when the size is not 1 and it is not known? That would avoid unnecessary iterations. I will of course only do that if profiling shows that it would have an impact.

Should we add some kind of validation to make sure that it is always the case? A future change might accidentally break that.

It seems like it would always be better to use the overload that takes the type.

Yeah. I just aligned the code as a first step so the intent is clear and it is easier to refactor.

There can be multiple known values of different types on a token and with the exception of int, they can be in any order.

Good to know. So I guess there is no need for an intermediate step and this can simply be cleaned up.

What intermediate step?

Could that assumption be used to simplify the lookup? A getKnownIntValue() which will bail out when the size is not 1 and it is not known?

The size is not guaranteed to be 1. There could be other values, such as known from other types or possible values, etc.

For known int value you don't need iteration, you can just check the first element.

It seems like it would always be better to use the overload that takes the type.

Yeah. I just aligned the code as a first step so the intent is clear and it is easier to refactor.

The overload without a type doesn't express intent and very likely its a bug in the code.

firewave · 2025-01-27T12:11:20Z

This should be mainly done as a first step.

I tried using getKnownValue() to avoid the duplicated fetching of the value but in many cases that wrecks the readability of the code so I am not sure if we should go with that (for now).

We should also keep the asserts to make sure that we do not introduce code which missed the preceding hasKnownIntValue() checks.

I will still profile to see if this doesn't introduce any regressions.

danmar

I am fine with this. if @pfultz2 @chrchr-github are happy then feel free to merge..

danmar · 2025-03-18T10:37:14Z

-                    const ValueFlow::Value& v = arg->values().front();
-                    result.intvalue = v.intvalue;
-                    result.errorPath.insert(result.errorPath.end(), v.errorPath.cbegin(), v.errorPath.cend());
+                    const ValueFlow::Value* v = arg->getKnownValue(ValueFlow::Value::ValueType::INT);


I don't have a strong opinon but well the previous hasKnownIntValue() check could be removed. and then we can check v != nullptr instead.. do it as you want..

That check is indeed no longer necessary. The getKnownValue() changes were done after this change and I did not check the code if it could be simplified. Will do so.

pfultz2 · 2025-03-22T04:06:47Z

+        return nullptr;
+    auto it = std::find_if(mImpl->mValues->begin(), mImpl->mValues->end(), std::mem_fn(&ValueFlow::Value::isKnown));
+    return it == mImpl->mValues->end() ? nullptr : &*it;
+}


I don't think we should have such a function. It could lead to bugs where a different valueflow type is returned.

Agreed.

Fortunately the subsequent cleanups made it unused and it can be removed.

…in `getKnownValue()`

firewave · 2025-03-22T11:04:42Z

There are more possible cleanups but I chose only those which did not require the code to be re-flowed.

firewave · 2025-03-22T11:05:19Z

            }
-            if (idx->hasKnownIntValue() && idx->getKnownIntValue() == 0)
-                continue;


I filed https://trac.cppcheck.net/ticket/13732 about detecting this.

firewave force-pushed the knownint branch 2 times, most recently from 3ac7fcc to 516a7f3 Compare January 5, 2025 12:23

danmar reviewed Jan 5, 2025

View reviewed changes

This comment was marked as outdated.

Sign in to view

firewave changed the title ~~fixed #13513 - corrected access of known int values~~ refs #13513 - adjusted access of known int values Jan 18, 2025

This comment was marked as outdated.

Sign in to view

firewave force-pushed the knownint branch from 516a7f3 to b4f0d28 Compare January 18, 2025 15:16

This comment was marked as outdated.

Sign in to view

firewave force-pushed the knownint branch from b4f0d28 to e89c24a Compare January 18, 2025 16:32

This comment was marked as outdated.

Sign in to view

firewave commented Jan 18, 2025

View reviewed changes

firewave force-pushed the knownint branch from e89c24a to 883aee6 Compare January 18, 2025 18:42

pfultz2 reviewed Jan 19, 2025

View reviewed changes

firewave force-pushed the knownint branch from 883aee6 to 7a7a8f3 Compare January 19, 2025 15:21

firewave changed the title ~~refs #13513 - adjusted access of known int values~~ fixed #13513 - adjusted access of known int values Jan 19, 2025

firewave force-pushed the knownint branch from 7a7a8f3 to e5986ed Compare January 19, 2025 16:46

firewave mentioned this pull request Jan 26, 2025

programmemory.cpp: avoid repeated iteration over values in Executor::executeImpl() #6701

Draft

firewave force-pushed the knownint branch from e5986ed to 86de5f8 Compare January 27, 2025 12:11

firewave force-pushed the knownint branch from 86de5f8 to 8694c69 Compare February 6, 2025 16:53

firewave marked this pull request as ready for review February 6, 2025 17:01

firewave force-pushed the knownint branch from 8694c69 to 19cd3ca Compare March 16, 2025 19:57

danmar approved these changes Mar 18, 2025

View reviewed changes

firewave marked this pull request as draft March 18, 2025 10:46

firewave force-pushed the knownint branch from 19cd3ca to 1a638cf Compare March 18, 2025 11:01

firewave marked this pull request as ready for review March 18, 2025 11:02

chrchr-github approved these changes Mar 19, 2025

View reviewed changes

pfultz2 reviewed Mar 22, 2025

View reviewed changes

firewave added 2 commits March 22, 2025 11:35

fixed #13513 - adjusted access of known int values

e82940d

symboldatabase.cpp: split logic in Variable::arrayDimensions()

df236fa

firewave added 7 commits March 22, 2025 11:35

Token: simplified hasKnownIntValue()

3a0f6f9

checkcondition.cpp: added missing hasKnownIntValue() check

ce4a674

valueflow.cpp: adjusted checks in valueFlowRightShift()

909021e

Token: added special handling for ValueFlow::Value::ValueType::INT …

104cd58

…in `getKnownValue()`

avoid some unnecessary Token::hasKnownIntValue() calls

7b26c1d

astutils.cpp: fixed warning about shadowed variable

10198b3

Token: removed unused getKnownValue()

5323ae8

firewave force-pushed the knownint branch from 1a638cf to 5323ae8 Compare March 22, 2025 10:36

firewave added 2 commits March 22, 2025 11:53

checkbufferoverrun.cpp: removed duplicated condition

3123142

avoid more unnecessary Token::hasKnownIntValue() calls

92547ea

firewave commented Mar 22, 2025

View reviewed changes

pfultz2 approved these changes Mar 22, 2025

View reviewed changes

firewave merged commit 37f4de2 into cppcheck-opensource:main Mar 22, 2025
60 checks passed

firewave deleted the knownint branch March 22, 2025 16:49

Conversation

firewave commented Jan 5, 2025

Uh oh!

danmar left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

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!

firewave commented Jan 27, 2025

Uh oh!

danmar left a comment

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 Mar 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants