From 3c4b6c87d73945be559c3a8d7ccd49f51bfea417 Mon Sep 17 00:00:00 2001 From: liuzhenlong Date: Sun, 12 Jul 2026 18:59:23 +0800 Subject: [PATCH] refactor: prefer std::make_unique and std::make_shared to direct use of new. --- .../code-review/references/custom-code-style.md | 17 +++++++++++++++++ xllm/core/distributed_runtime/comm_channel.cpp | 2 +- .../framework/xtensor/xtensor_dist_client.cpp | 2 +- xllm/core/triton_jit/src/jit_kernel.cpp | 6 +++--- xllm/core/util/suffix_tree.cpp | 4 ++-- 5 files changed, 24 insertions(+), 7 deletions(-) diff --git a/.agents/skills/code-review/references/custom-code-style.md b/.agents/skills/code-review/references/custom-code-style.md index 23ca1ca78f..aabd6f29a8 100644 --- a/.agents/skills/code-review/references/custom-code-style.md +++ b/.agents/skills/code-review/references/custom-code-style.md @@ -168,6 +168,23 @@ virtual ModelOutput forward(torch::Tensor tokens, ...); - Use `std::shared_ptr` only when shared ownership is genuinely needed. - Raw pointers are acceptable only for non-owning references where the lifetime is clearly managed elsewhere. +- **Prefer `std::make_unique` and `std::make_shared` to direct use of `new`** when creating heap objects managed by smart pointers. + - Avoid repeating the type and avoid exception-unsafe patterns such as passing `std::shared_ptr(new T(...))` in the same statement as other expressions. + - Direct `new` is acceptable when make functions cannot be used (e.g. custom deleters, braced initialization of the pointee, or APIs that require a raw pointer and take ownership explicitly). + - If `new` is required, assign the result to a smart pointer in a standalone statement before any other expression that may throw. + +```cpp +// Good +auto widget = std::make_unique(arg); +auto widget = std::make_shared(arg); +stub_ = std::make_unique(&channel_); + +// Bad +std::unique_ptr widget(new Widget(arg)); +stub_.reset(new ServiceStub(&channel_)); +process(std::shared_ptr(new Widget), compute_priority()); // may leak +``` + --- ## 6. Scoping & Visibility diff --git a/xllm/core/distributed_runtime/comm_channel.cpp b/xllm/core/distributed_runtime/comm_channel.cpp index 7f63f59027..c9a3518b59 100644 --- a/xllm/core/distributed_runtime/comm_channel.cpp +++ b/xllm/core/distributed_runtime/comm_channel.cpp @@ -35,7 +35,7 @@ bool CommChannel::init_brpc(const std::string& server_address) { return false; } - stub_.reset(new proto::DistributeWorker_Stub(&channel_)); + stub_ = std::make_unique(&channel_); return true; } diff --git a/xllm/core/framework/xtensor/xtensor_dist_client.cpp b/xllm/core/framework/xtensor/xtensor_dist_client.cpp index babeaa6ab1..736c6a4aa5 100644 --- a/xllm/core/framework/xtensor/xtensor_dist_client.cpp +++ b/xllm/core/framework/xtensor/xtensor_dist_client.cpp @@ -38,7 +38,7 @@ XTensorDistClient::XTensorDistClient(int32_t global_rank, LOG(ERROR) << "Failed to initialize brpc channel to " << server_address; return; } - stub_.reset(new proto::XTensorDist_Stub(&channel_)); + stub_ = std::make_unique(&channel_); wait_for_server_ready(server_address); } diff --git a/xllm/core/triton_jit/src/jit_kernel.cpp b/xllm/core/triton_jit/src/jit_kernel.cpp index 4cc98ae01a..2201ac74d4 100644 --- a/xllm/core/triton_jit/src/jit_kernel.cpp +++ b/xllm/core/triton_jit/src/jit_kernel.cpp @@ -92,9 +92,9 @@ JITKernel& JITKernel::get(std::string py_path, std::string fn_name) { std::lock_guard lock(registry_mutex); auto it = registry.find(key); if (it == registry.end()) { - std::unique_ptr p( - new JITKernel(std::move(py_path), std::move(fn_name))); - auto [ins, ok] = registry.emplace(key, std::move(p)); + auto [ins, ok] = registry.emplace( + key, + std::make_unique(std::move(py_path), std::move(fn_name))); it = ins; } return *it->second; diff --git a/xllm/core/util/suffix_tree.cpp b/xllm/core/util/suffix_tree.cpp index 7eb5f14870..e16a14bdaa 100644 --- a/xllm/core/util/suffix_tree.cpp +++ b/xllm/core/util/suffix_tree.cpp @@ -375,7 +375,7 @@ void SuffixTree::append(int32_t seq_id, int32_t token) { // This should be the first child being added. assert(!node->head_child && !node->tail_child); node->head_child = node->tail_child = new_child; - new_child->group.reset(new Group(new_child)); + new_child->group = std::make_shared(new_child); } else { assert(node->tail_child); insert_into_siblings_after(new_child, node->tail_child); @@ -500,7 +500,7 @@ void SuffixTree::append(int32_t seq_id, int32_t token) { // Create a new group for the child node. new_node->head_child = new_node->tail_child = child; - child->group.reset(new Group(child)); + child->group = std::make_shared(child); // Increment the new node count and update siblings lists. increment_count(new_node);