Skip to content
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

reduced Tokenizer::isCPP() usage / SymbolDatabase: removed mIsCpp and isCPP() #5725

Merged
merged 5 commits into from
Mar 10, 2024

Conversation

firewave
Copy link
Collaborator

@firewave firewave commented Dec 4, 2023

Each Token (should) be connected to a TokenList. Tokenizer just encapsulates that so we have no need to check the Tokenizer but can simply ask the Token.

Also if we have function calls we pass in a flag to tell it if it is C/C++ we can get rid of that flag and simply ask the Token. In several cases we were only passing that flag to be used in some underlying calls making it completely unnecessary in the top call.

@firewave firewave changed the title reduced usage of Tokenizer::isCPP() reduced Tokenizer::isCPP() usage Dec 4, 2023
@firewave firewave added the merge-after-next-release Wait with merging this PR until after the next Release label Dec 4, 2023
@firewave
Copy link
Collaborator Author

firewave commented Dec 4, 2023

This might also speed up processing C code a bit as we previously forcefully used the C++ handling where it might not have been appropriate.

We should merge this after the next release so the impact of the changes might be visible instead of getting lost in months of accumulated differences.

@firewave
Copy link
Collaborator Author

firewave commented Dec 4, 2023

The remaining usages which could be removed is in conjunction with types that wrap a token - and those are sparse.

I also think we could switch from Tokenizer to TokenList in Check and possibly other classes but that is for a later date and #5323 should be merged before that.

@firewave
Copy link
Collaborator Author

firewave commented Dec 4, 2023

The tests which failed were added in ff9c04d.

Looking at what they were testing for I wonder if these failures might be somewhat related to some regressions which occurred in 2.6:
https://trac.cppcheck.net/ticket/11199
https://trac.cppcheck.net/ticket/11200
https://trac.cppcheck.net/ticket/11201

@firewave firewave marked this pull request as draft December 4, 2023 23:27
@firewave
Copy link
Collaborator Author

firewave commented Dec 5, 2023

Further cleanups revealed potential issues/cases with undetermined standards. Those need to be resolved beforehand. Unfortunately that existing handling is quite awkward (surprise) so a fix is not straight forward and I currently can't wrap around it... It potentially might also require some other in-progress stuff to land first...

@firewave
Copy link
Collaborator Author

Requires #5853 to be merged first.

@@ -53,7 +53,7 @@
//---------------------------------------------------------------------------

SymbolDatabase::SymbolDatabase(Tokenizer& tokenizer, const Settings& settings, ErrorLogger* errorLogger)
: mTokenizer(tokenizer), mSettings(settings), mErrorLogger(errorLogger), mIsCpp(mTokenizer.isCPP())
Copy link
Collaborator Author

@firewave firewave Feb 27, 2024

Choose a reason for hiding this comment

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

It seems like this could have been left in to avoid the repeated calls on the tokens but the language had not been determined at this point yet so this caused assertions in the unit tests.

I also think it is cleaner to have one less place which provides this information.

@firewave firewave changed the title reduced Tokenizer::isCPP() usage reduced Tokenizer::isCPP() usage and some additional cleanups related to it Feb 27, 2024
@firewave firewave changed the title reduced Tokenizer::isCPP() usage and some additional cleanups related to it reduced Tokenizer::isCPP() usage / SymbolDatabase: removed mIsCpp and isCPP() Feb 27, 2024
@firewave firewave force-pushed the tokenizer-cpp branch 3 times, most recently from c7d6290 to 80a2077 Compare March 5, 2024 10:32
@firewave firewave marked this pull request as ready for review March 5, 2024 10:32
@firewave firewave force-pushed the tokenizer-cpp branch 2 times, most recently from c72a06b to 2218041 Compare March 5, 2024 12:34
@firewave firewave merged commit 40cc208 into danmar:main Mar 10, 2024
64 checks passed
@firewave firewave deleted the tokenizer-cpp branch March 10, 2024 23:36
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