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

aligned and optimized unique error handling #5280

Merged
merged 11 commits into from
Dec 17, 2023
Merged

Conversation

firewave
Copy link
Collaborator

@firewave firewave commented Jul 31, 2023

The handling in CppCheck::reportErr() and Executor::hasToLog() was slightly different. I hope this can somehow be shared after the executor reworking.

We were also using a very inappropriate container for the error list which caused a lot of overhead.

-D__GNUC__ --debug-warnings --template=daca2 --check-library -j2 ../test/testsymboldatabase.cpp

Clang 15
main process 284,218,587 -> 175,691,241
worker process 9,123,697,183 -> 8,951,903,360

@firewave
Copy link
Collaborator Author

I will tackle the impact of ErrorMessage::toString() in another PR.

@firewave
Copy link
Collaborator Author

firewave commented Aug 7, 2023

Still needs some unit tests (in case they don't exist yet).

cli/executor.h Show resolved Hide resolved
@firewave firewave force-pushed the hastolog branch 2 times, most recently from 3b908b2 to bd0cec3 Compare August 14, 2023 22:23
@firewave
Copy link
Collaborator Author

firewave commented Aug 14, 2023

This is madness - we check for unique messages in three different places. Hopefully the ErrorLogger usage untangling and executor reworking will get rid of that.

@firewave
Copy link
Collaborator Author

I still want to try to add unit tests for the code I changed.

@firewave firewave force-pushed the hastolog branch 2 times, most recently from 2767b0d to 2cfb0a4 Compare December 2, 2023 15:05
@firewave
Copy link
Collaborator Author

firewave commented Dec 2, 2023

Some of the additional tests are quite blackbox-y and seem redundant but since the logic is all over the place at the moment we need those test to make sure that logic is in place before we move it in the future.

@firewave
Copy link
Collaborator Author

This has sufficient testing now.

I also finally have a better understanding how the different ErrorLogger instances chain together which should allow for some further cleanups in the next step.

@firewave firewave marked this pull request as ready for review December 17, 2023 18:15
@firewave firewave force-pushed the hastolog branch 3 times, most recently from c57c98d to 9f29c4b Compare December 17, 2023 18:46
@firewave firewave merged commit aa7629d into danmar:main Dec 17, 2023
68 checks passed
@firewave firewave deleted the hastolog branch December 17, 2023 20:59
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