Cleanup: Fix clang-tidy for performance-unnecessary-value-param#1024
Cleanup: Fix clang-tidy for performance-unnecessary-value-param#1024rlazarus wants to merge 2 commits 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.
|
oops, looks like one test is failing ;) |
|
Don't merge -- asan/tsan are failing, so it's possible one of these changes wasn't actually as safe as it looked. Trying to sort that out. |
|
yup, was not going to merge... |
|
Unfortunately I'm heading out on vacation -- I was hoping to have this figured out by the end of today, but here we are. I'll be back on Thursday, but I'm going to close the PR while I'm OOO since it's hardly urgent. I'll open a fresh one once I figure out why this breaks asan and tsan. (Or, if someone else sees it, feel free to fix in my absence -- just let me know, because I'm curious!) |
| ~MockDetector(); | ||
|
|
||
| void runCallbacks(HostSharedPtr host) { | ||
| void runCallbacks(const HostSharedPtr& host) { |
There was a problem hiding this comment.
This is the troublesome line. By using a reference here the "host" variable refers back to what's passed into runCallbacks in sds_test.cc:244 rather than making a shared_ptr copy and thus reference count increase. One of the callbacks modifies the host list returned by cluster_->hosts(), with this change the reference count indicates it is safe to free the host when the host list is updated. The callback itself DOES use pass-by-value function argument, so when it returns the HostSharedPtr goes out of scope resulting in a read of the reference count (but the object has already been deleted).
It can be const, but not a reference.
There was a problem hiding this comment.
Aha! Thanks for tracking that down.
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.
Description: Adds unit-level coverage to the JvmBridgeUtility. Signed-off-by: Mike Schore <mike.schore@gmail.com> Signed-off-by: JP Simard <jp@jpsim.com>
Description: Adds unit-level coverage to the JvmBridgeUtility. Signed-off-by: Mike Schore <mike.schore@gmail.com> Signed-off-by: JP Simard <jp@jpsim.com>
**Description** This PR add error response handling for GCP Gemini models. It converts error message from GCP Gemini format to openai api format **Related Issues/PRs (if applicable)** Issue: envoyproxy/ai-gateway#609 --------- Signed-off-by: Sukumar Gaonkar <sgaonkar4@bloomberg.net>
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.