Skip to content

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

Draft
firewave wants to merge 2 commits intocppcheck-opensource:mainfrom
firewave:pm-execute
Draft

programmemory.cpp: avoid repeated iteration over values in Executor::executeImpl()#6701
firewave wants to merge 2 commits intocppcheck-opensource:mainfrom
firewave:pm-execute

Conversation

@firewave
Copy link
Copy Markdown
Collaborator

No description provided.

@firewave
Copy link
Copy Markdown
Collaborator Author

-D__GNUC__ --check-level=exhaustive ../lib/utils.cpp

Clang 17 722,148,610 -> 709,134,445
GCC 14 763,758,550 -> 748,797,242

The example from https://trac.cppcheck.net/ticket/10765#comment:4:

Clang 17 3,745,218,263 -> 3,683,364,401
GCC 14 4,007,836,001 -> 3,916,329,290

@firewave
Copy link
Copy Markdown
Collaborator Author

@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.

danmar
danmar previously approved these changes Aug 16, 2024
Copy link
Copy Markdown
Collaborator

@danmar danmar left a 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

@pfultz2
Copy link
Copy Markdown
Contributor

pfultz2 commented Aug 16, 2024

Tokens can have more than one known value, but its not very common.

Comment thread lib/programmemory.cpp
}
default:
break;
}
Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Collaborator Author

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.

@firewave
Copy link
Copy Markdown
Collaborator Author

Tokens can have more than one known value, but its not very common.

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

@firewave firewave marked this pull request as draft September 17, 2024 12:18
@firewave
Copy link
Copy Markdown
Collaborator Author

Requires #7181 to be merged first.

There are multiple known values. TestAutoVariables::danglingLifetimeContainer fails when I assert on that. It has known ITERATOR_START and CONTAINER_SIZE.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants