Copilot commented on code in PR #3417: URL: https://github.com/apache/brpc/pull/3417#discussion_r3670584928
########## test/brpc_bvar_mutex_unittest.cpp: ########## @@ -0,0 +1,361 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// brpc - A framework to host and access services throughout Baidu. + +// Date: Mon Jul 27 15:58:00 CST 2026 + +#include <atomic> +#include <csignal> +#include <cstring> +#include <sys/wait.h> +#include <unistd.h> + +#include <gflags/gflags.h> +#include <gtest/gtest.h> + +#include "bthread/bthread.h" +#include "butil/compat.h" +#include "butil/time.h" +#include "bvar/bvar.h" Review Comment: The test re-execs the current binary via "/proc/self/exe", which is Linux-specific and will fail on macOS (and other non-Linux platforms). Consider using butil::GetProcessAbsolutePath() to obtain a portable executable path before calling execl(). This issue also appears on line 301 of the same file. ########## src/bthread/types.h: ########## @@ -197,6 +197,13 @@ typedef struct bthread_mutex_t { mutex_owner_t owner; } bthread_mutex_t; +typedef struct { + bthread_mutex_t mutex; + uint64_t owner; + uint32_t owner_kind; + uint32_t recursion; +} bthread_recursive_mutex_t; Review Comment: bthread_mutex_t (and several other bthread primitives in this header) provide a C++ default constructor to put the struct into a known initial state when used from C++. bthread_recursive_mutex_t currently lacks that, making accidental use of an uninitialized instance easier. Consider adding a C++ default constructor and DISALLOW_COPY_AND_ASSIGN, consistent with the surrounding types. ########## BUILD.bazel: ########## @@ -370,6 +370,23 @@ cc_library( }), ) +cc_library( + # Header-only interface so :bvar can use RecursiveMutex without depending on + # :bthread (which itself depends on :bvar). Implementation lives in :bthread. + name = "bthread_recursive_mutex", + hdrs = [ + "src/bthread/recursive_mutex.h", + "src/bthread/types.h", + ], + includes = [ + "src/", + ], + visibility = ["//visibility:public"], + deps = [ + ":butil", + ], +) Review Comment: In Bazel, //:bvar now instantiates bthread::RecursiveMutex (src/bvar/variable.cpp) but its implementation symbols (bthread_recursive_mutex_*) live in //:bthread. Since //:bvar cannot depend on //:bthread (cycle), any external target that depends on //:bvar without also linking //:bthread will hit undefined symbols at link time, which is a breaking change relative to the prior standalone bvar. Consider moving the recursive-mutex implementation into a separate cc_library that both //:bvar and //:bthread can depend on (and that does not depend on //:bvar), or otherwise documenting/enforcing the required link dependency. -- 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]
