-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
programmemory.cpp: avoid repeated iteration over values in Executor::executeImpl()
#6701
base: main
Are you sure you want to change the base?
Conversation
Clang 17 The example from https://trac.cppcheck.net/ticket/10765#comment:4: Clang 17 |
@pfultz2 Is this a valid change? The previous code had priority which value type have been taken which is no longer the case in the new code. Tests are still passing and I also added an else to break into and that was never hit so it seems like this change should be fine. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% sure but as far as I know a token can only have 1 known value so then this should be safe.. but let's see what @pfultz2 says also
Tokens can have more than one known value, but its not very common. |
} | ||
default: | ||
break; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to make this a function that returns a pointer to the value. Then we can just write if((const ValueFlow::Value* value = findKnownValue(expr->values())))
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense.
But then we would need to exclude the INT
handling since that currently has priority and a different handling.
So the priority of fetching the type is important? What is such a case? We should add a unit test for that (if there isn't one already). |
No description provided.