-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Fix cppcheck performance checks #2371
base: master
Are you sure you want to change the base?
Conversation
Please no whole clang-reformat of source files mixed with code changes, this makes everything very difficult to review. It may be desirable to reformat the whole source tree with a single clang-format pass, but doing so should probably be done after in a single isolated PR, after announcing it on the mailing list to warn anyone that has an active PRs to rebase their changes on top of it after it is submitted. Other large scale changes (e.g. removing What is the purpose of this patch? in particular what cppcheck performance check is this supposed to address? Also, what is the practical benefit here? Performance optimization should be justified with numbers that show they improve a measurable metric for reasonable use cases. E.g. it looks like you somehow optimize ElideMiddle() by reserving a string buffer but that probably doesn't impact Ninja build times in any reasonable way, so I don't see a reason to accept your PR here unless you can show that this improves things for users, instead of a small micro-optimization. |
@digit-google First of all, thank you for the feedback. I was not aware of I would certainly be willing to do both a full clang-format pass as well as remove all instances of Finally, I edited the PR description. Please tell me if you need more. |
Thanks for your changes, a few more notes though:
To be honest, I don't think your changes have significant value, though I am not opposed to them (after proper fixes), but @jhasse who is the official Ninja maintainer may have its own opinion on this. Regarding stylistic large-scale changes, I think @jhasse, prefers to avoid them for the moment, as their benefits are low compared to their drawbacks. |
I think the The In general I would suggest to uninstall cppcheck and never use it. Use clang-tidy instead. |
Also clang-formatted the edited files.
Edit:
For the latter, after reading what it would be with
result.replace(...)
, it seemed better to limit allocations and copies by reserving the lengthwidth
and appending the necessary strings.Used
git clang-format
rather thanclang-format
entire files.