Copilot commented on code in PR #3700:
URL: https://github.com/apache/brpc/pull/3700#discussion_r4442125237
##########
src/bvar/detail/combiner.h:
##########
@@ -205,17 +205,30 @@ friend class GlobalValue<self_type>;
//
// NOTE: Only available to non-atomic types.
template <typename Op>
- void merge_global(const Op &op, self_shared_type& c) {
- const self_shared_type& c_ref = nullptr != c ? c : combiner.lock();
- if (nullptr != c_ref) {
- GlobalValue<self_type> g(this, c_ref.get());
- element.merge_global(op, g);
+ void merge_global(const Op &op, const self_shared_type& c) {
+ // The caller keeps c alive during the merge. A conditional
+ // expression mixing c and combiner.lock() would copy c into a
+ // temporary shared_ptr, even when c is non-null.
+ if (c != nullptr) {
Review Comment:
Since `self_shared_type` is a `shared_ptr`, using `if (c)` is the idiomatic
and most portable way to test for non-null ownership (it uses the safe
`operator bool`). This is a small readability improvement and avoids depending
on `nullptr` comparison overloads.
##########
src/bvar/detail/combiner.h:
##########
@@ -205,17 +205,30 @@ friend class GlobalValue<self_type>;
//
// NOTE: Only available to non-atomic types.
template <typename Op>
- void merge_global(const Op &op, self_shared_type& c) {
- const self_shared_type& c_ref = nullptr != c ? c : combiner.lock();
- if (nullptr != c_ref) {
- GlobalValue<self_type> g(this, c_ref.get());
- element.merge_global(op, g);
+ void merge_global(const Op &op, const self_shared_type& c) {
+ // The caller keeps c alive during the merge. A conditional
+ // expression mixing c and combiner.lock() would copy c into a
+ // temporary shared_ptr, even when c is non-null.
+ if (c != nullptr) {
+ merge_global_impl(op, c.get());
+ return;
+ }
+ self_shared_type hold = combiner.lock();
+ if (hold != nullptr) {
+ merge_global_impl(op, hold.get());
}
}
ElementContainer<ElementTp> element;
private:
friend class AgentCombiner<ResultTp, ElementTp, BinaryOp>;
+
+ template <typename Op>
+ void merge_global_impl(const Op& op, self_type* c) {
+ GlobalValue<self_type> g(this, c);
+ element.merge_global(op, g);
+ }
Review Comment:
`merge_global_impl` takes a raw pointer that is expected to be non-null (all
current call sites ensure that). Making this a reference parameter (e.g.,
`self_type&`) would better encode the non-null contract and prevent accidental
null calls in future refactors. This would also simplify `GlobalValue`
construction by passing `&c`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
