Skip to content

fixed #11342 - cache Library::getFunctionName() calls in Token #7573

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

firewave
Copy link
Collaborator

@firewave firewave commented Jun 5, 2025

No description provided.

@firewave
Copy link
Collaborator Author

firewave commented Jun 5, 2025

cppcheck: build/token.cpp:3128: const std::string &Token::funcname(const Library &) const: Assertion `*mImpl->mFuncName == library.getFunctionName(this)' failed.
Internal error: cppcheck received signal SIGABRT - abort or assertion
Callstack:
#0  0x562da7e19d1c in Token::funcname[abi:cxx11](Library const&) const
#1  0x562da7d8cb93 in Library::returnValueType[abi:cxx11](Token const*) const
#2  0x562da7bb086e in SymbolDatabase::setValueTypeInTokenList(bool, Token*)
#3  0x562da7ba70bb in SymbolDatabase::SymbolDatabase(Tokenizer&, Settings const&, ErrorLogger&)

Library::returnValueType() checks if the given token isNotLibraryFunction(). This looks wrong to me because I would expect that the given token is supposed to be a type. Or is the wrong token passed into this?

@firewave
Copy link
Collaborator Author

firewave commented Jun 5, 2025

Library::returnValueType() checks if the given token isNotLibraryFunction(). This looks wrong to me because I would expect that the given token is supposed to be a type. Or is the wrong token passed into this?

Still that shouldn't produce different results on subsequent calls on the same token. Since the library is immutable it seems like this is caused by some tokenizing/simplification.

@firewave
Copy link
Collaborator Author

firewave commented Jun 5, 2025

The problem is that the astParent() is set on later calls.

From TestClass::memsetOnClass:

000001FE7416C2E0 astParent: 0000000000000000 previous: 000001FE7416DBA0 size
[...]
000001FE7416C2E0 astParent: 000001FE7416DBA0 previous: 000001FE7416DBA0 size std::vector::size
Assertion failed: *mImpl->mFuncName == library.getFunctionName(this), file S:\GitHub\cppcheck-fw\lib\token.cpp, line 273

@firewave firewave force-pushed the fname-cache branch 2 times, most recently from b1f1df1 to 2cee634 Compare June 5, 2025 22:44
if (!mImpl->mFuncName)
mImpl->mFuncName = new std::string(library.getFunctionName(this));
const std::string fname = library.getFunctionName(this);
std::cout << this << " astParent: " << astParent() << " previous: " << previous() << " " << stringify(stringifyOptions::forDebug()) << " " << fname << std::endl;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume that this is debug code that will be removed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still WIP - I forgot to mark it a draft.

@danmar
Copy link
Owner

danmar commented Jun 9, 2025

I assume there is speedup but do you see it also?

@firewave firewave marked this pull request as draft June 9, 2025 13:58
@firewave
Copy link
Collaborator Author

I assume there is speedup but do you see it also?

Since it is still a draft (and not working as expected) I did not provide performance data yet. It is about 2-4% of the total Ir - that is also mentioned in the ticket.

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.

2 participants