Copilot commented on code in PR #3033:
URL: https://github.com/apache/brpc/pull/3033#discussion_r2217330280
##########
src/bthread/task_control.cpp:
##########
@@ -480,14 +482,11 @@ void TaskControl::signal_task(int num_task, bthread_tag_t
tag) {
num_task = 2;
}
auto& pl = tag_pl(tag);
- int start_index = butil::fmix64(pthread_numeric_id()) % PARKING_LOT_NUM;
- num_task -= pl[start_index].signal(1);
- if (num_task > 0) {
- for (int i = 1; i < PARKING_LOT_NUM && num_task > 0; ++i) {
- if (++start_index >= PARKING_LOT_NUM) {
- start_index = 0;
- }
- num_task -= pl[start_index].signal(1);
+ int start_index = butil::fmix64(pthread_numeric_id()) %
_pl_num_of_each_tag;
+ for (int i = 0; i < _pl_num_of_each_tag && num_task > 0; ++i) {
Review Comment:
The loop logic is incorrect. The original code first signaled one parking
lot, then conditionally signaled others if num_task > 0. Now the first signal
is inside the loop, but the condition check and increment logic that follows
should be inside the loop body, not after the signal call.
```suggestion
for (int i = 0; i < _pl_num_of_each_tag; ++i) {
if (num_task <= 0) {
break;
}
```
##########
src/bthread/task_control.cpp:
##########
@@ -480,14 +482,11 @@ void TaskControl::signal_task(int num_task, bthread_tag_t
tag) {
num_task = 2;
}
auto& pl = tag_pl(tag);
- int start_index = butil::fmix64(pthread_numeric_id()) % PARKING_LOT_NUM;
- num_task -= pl[start_index].signal(1);
- if (num_task > 0) {
- for (int i = 1; i < PARKING_LOT_NUM && num_task > 0; ++i) {
- if (++start_index >= PARKING_LOT_NUM) {
- start_index = 0;
- }
- num_task -= pl[start_index].signal(1);
+ int start_index = butil::fmix64(pthread_numeric_id()) %
_pl_num_of_each_tag;
+ for (int i = 0; i < _pl_num_of_each_tag && num_task > 0; ++i) {
+ num_task -= pl[start_index].signal(1);
+ if (++start_index >= _pl_num_of_each_tag) {
+ start_index = 0;
Review Comment:
The signal call and index increment logic should be properly structured
within the loop. The current code has the signal call and index management at
the same indentation level, which breaks the original logic flow.
```suggestion
if (num_task > 0) {
if (++start_index >= _pl_num_of_each_tag) {
start_index = 0;
}
```
##########
src/bthread/bthread.cpp:
##########
@@ -216,7 +218,7 @@ static bool validate_bthread_current_tag(const char*,
int32_t val) {
return false;
}
BAIDU_SCOPED_LOCK(bthread::g_task_control_mutex);
- auto c = bthread::get_task_control();
+ auto c = get_task_control();
Review Comment:
This namespace cleanup change (removing 'bthread::' prefix) is unrelated to
the parking lot customization feature and should be in a separate commit or PR
to maintain clear change isolation.
```suggestion
auto c = bthread::get_task_control();
```
--
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]
