-
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
avoid duplicated evaluation in ProgramMemoryState::removeModifiedVars()
#6628
Conversation
Clang 18 @pfultz2 please have a look |
ProgramMemoryState::removeModifiedVars()
ProgramMemoryState::removeModifiedVars()
This should help with https://trac.cppcheck.net/ticket/12271. |
Looks like this (anti-) pattern also exists in |
Yes, I did not touch those because they are not in the hot path. Well one of them shows up in the profiler but changing it let to strange results with one of the compilers so I left it for now. |
lib/programmemory.cpp
Outdated
{ | ||
if (!condition) | ||
return false; | ||
MathLib::bigint result = 0; |
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.
To be on the safe side, we should initialize result
before the first return.
lib/programmemory.cpp
Outdated
return {1}; | ||
if (result == 0) | ||
return {0}; | ||
} |
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 just call execute and use the isTrue
and isFalse
functions:
auto result = execute(cond, pm, *settings);
if(isTrue(result))
return {1};
if(isFalse(result))
return {0};
return {};
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.
Then there is no need to refactor the evaluateCondition
function.
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.
And this same code could be used in evaluateInt
as well, we just need to expose the isTrue
and isFalse
functions.
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.
Done. As mentioned before I will not touch other code until I encounter it in a profile.
…::removeModifiedVars()` Co-authored-by: Paul Fultz II <[email protected]>
No description provided.