fix(lint-configs): Suppress clang-tidy copy warnings for std::shared_ptr as the copy is necessary for reference counting.#115
Open
davidlion wants to merge 1 commit into
Open
Conversation
Contributor
WalkthroughThe ChangesLint configuration update
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
std::shared_ptr from clang-tidy unnecessary copy checks as skipping the copy will skip the reference counting.std::shared_ptr as the copy is necessary for reference counting.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The clang-tidy checks
unnecessary-copy-initializationandunnecessary-value-paramhelp devs to avoid unnecessary copies by pointing out when it is possible to useconst&. This is great most of the time, but these checks will also flag reference counting types.In our code this issue manifests primarily with
std::shared_ptrfunction parameters. When the shared pointer would be copy constructed the reference count would be increased, however if the dev switches tostd::shared_ptr const&there will be no reference counting as there is no construction. It is technically safe to do this as long as the object being referenced is guaranteed to not fall out of scope while the reference is held, but it is much easier to reason about reference counted types when they always perform the counting.If performing these copies is ever a serious performance concern we should probably be investigating how to not use shared_ptrs entirely than skipping the copy.
There are other referencing counting stdlib types that this applies to, but I think we can add them on a case-by-case basis.
https://clang.llvm.org/extra/clang-tidy/checks/performance/unnecessary-copy-initialization.html
https://clang.llvm.org/extra/clang-tidy/checks/performance/unnecessary-value-param.html
llvm/llvm-project#153247
Checklist
breaking change.
Validation performed
I've been building y-scope/clp/main and davidlion/clp/clpp with these changes.
Summary by CodeRabbit