cleanup: Fix clang-tidy for performance-unnecessary-value-param#1057
Closed
rlazarus wants to merge 1 commit intoenvoyproxy:masterfrom
Closed
cleanup: Fix clang-tidy for performance-unnecessary-value-param#1057rlazarus wants to merge 1 commit intoenvoyproxy:masterfrom
rlazarus wants to merge 1 commit intoenvoyproxy:masterfrom
Conversation
Wherever safe, avoid unnecessary expensive copies due to value-typed function parameters, either by making them const references or by using std::move. See http://clang.llvm.org/extra/clang-tidy/checks/performance-unnecessary-value-param.html for details. This is the same tool that I ran in envoyproxy#1024, but without touching HealthCheckerImplBase::runCallbacks: as @dpn points out, that copy is necessary for reference counting, and that's why envoyproxy#1024 broke asan/tsan.
Member
|
@rlazarus presumably you used some automated tool to do this. How did you indicate that something that was changed before should not be changed? In general passing shared pointers by reference is dangerous (as we saw here) and it makes me uneasy that the tool might be doing things we don't fully understand. |
Member
|
@rlazarus I'm going to close this for now. Let's reopen when we have some clarity on the ownership issue that was causing problems. |
jpsim
pushed a commit
that referenced
this pull request
Nov 28, 2022
In the case when the JVM calls a native method, the native method will automatically handle local references within the method upon a the method's return. However, in the case where the native method calls a JVM method, local references created will not be deleted after the method returns. In some of these cases, we have forgotten to clean up some of these local references prior to returning. While we previously did not do this clean up for `on_error` and `on_cancel` and since these were rarer events, we did not observe any issues. However, with the introduction of platform based filters, we also did not clean this up for the filter initialization which does occur more often and we ran into issues. Signed-off-by: Alan Chiu <achiu@lyft.com> For an explanation of how to fill out the fields, please see the relevant section in [PULL_REQUESTS.md](https://github.com/envoyproxy/envoy/blob/master/PULL_REQUESTS.md) Description: jni: fix unreleased local refs Risk Level: low Testing: ci Docs Changes: na Release Notes: na [Optional Fixes #Issue] [Optional Deprecated:] Signed-off-by: JP Simard <jp@jpsim.com>
jpsim
pushed a commit
that referenced
this pull request
Nov 29, 2022
In the case when the JVM calls a native method, the native method will automatically handle local references within the method upon a the method's return. However, in the case where the native method calls a JVM method, local references created will not be deleted after the method returns. In some of these cases, we have forgotten to clean up some of these local references prior to returning. While we previously did not do this clean up for `on_error` and `on_cancel` and since these were rarer events, we did not observe any issues. However, with the introduction of platform based filters, we also did not clean this up for the filter initialization which does occur more often and we ran into issues. Signed-off-by: Alan Chiu <achiu@lyft.com> For an explanation of how to fill out the fields, please see the relevant section in [PULL_REQUESTS.md](https://github.com/envoyproxy/envoy/blob/master/PULL_REQUESTS.md) Description: jni: fix unreleased local refs Risk Level: low Testing: ci Docs Changes: na Release Notes: na [Optional Fixes #Issue] [Optional Deprecated:] Signed-off-by: JP Simard <jp@jpsim.com>
mathetake
pushed a commit
that referenced
this pull request
Mar 3, 2026
…am from 1.6.11 to 1.7.0 (#1057)
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
2 participants
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.
Wherever safe, avoid unnecessary expensive copies due to value-typed function parameters, either by making them const references or by using std::move. See http://clang.llvm.org/extra/clang-tidy/checks/performance-unnecessary-value-param.html for details.
This is the same tool that I ran in #1024, but now without changing the
HostSharedPtrargument toHealthCheckerImplBase::runCallbacks()-- as @dnoe points out, that copy is necessary for reference counting, and that's why #1024 broke asan/tsan.