refactor(behavior_velocity_no_stopping_area_module): cppcheck warnings #6986

veqcc · 2024-05-12T04:48:07Z

Description

I have made several chages based on CppCheck warnings.
Some commits are irrelevant to CppCheck.

remove redundant if branch
e0e49be

planning/behavior_velocity_no_stopping_area_module/src/scene_no_stopping_area.cpp:94:13: style: Condition '!collision_points.empty()' is always true [knownConditionTrueFalse]
        if (!collision_points.empty()) {
            ^
planning/behavior_velocity_no_stopping_area_module/src/scene_no_stopping_area.cpp:90:35: note: Assuming that condition 'collision_points.empty()' is not redundant
        if (collision_points.empty()) {
                                  ^
planning/behavior_velocity_no_stopping_area_module/src/scene_no_stopping_area.cpp:94:13: note: Condition '!collision_points.empty()' is always true
        if (!collision_points.empty()) {
            ^

remove unused variable
021129a
remove redundant initialization
0eb34eb

planning/behavior_velocity_no_stopping_area_module/src/scene_no_stopping_area.cpp:333:20: style: Redundant initialization for 'ego_area_end_idx'. The initialized value is overwritten before it is read. [redundantInitialization]
  ego_area_end_idx = ego_area_start_idx;
                   ^
planning/behavior_velocity_no_stopping_area_module/src/scene_no_stopping_area.cpp:309:27: note: ego_area_end_idx is initialized
  size_t ego_area_end_idx = ego_area_start_idx;
                          ^
planning/behavior_velocity_no_stopping_area_module/src/scene_no_stopping_area.cpp:333:20: note: ego_area_end_idx is overwritten
  ego_area_end_idx = ego_area_start_idx;
                   ^

remove unused dist_from_area_sum variable
9e76436

planning/behavior_velocity_no_stopping_area_module/src/scene_no_stopping_area.cpp:338:26: style: Variable 'dist_from_area_sum' is assigned a value that is never used. [unreadVariable]
      dist_from_area_sum += tier4_autoware_utils::calcDistance2d(pp.at(i), pp.at(i - 1));
                         ^

Question

I have one question: there seems to be several unused functions in debug.cpp.
Is it intentional? If not, it looks it's better to delete them in terms of maintenance and readbility.

planning/behavior_velocity_no_stopping_area_module/src/debug.cpp:134:0: style: The function 'createDebugMarkerArray' is never used. [unusedFunction]
visualization_msgs::msg::MarkerArray NoStoppingAreaModule::createDebugMarkerArray()
^
planning/behavior_velocity_no_stopping_area_module/src/debug.cpp:165:0: style: The function 'createVirtualWalls' is never used. [unusedFunction]
motion_utils::VirtualWalls NoStoppingAreaModule::createVirtualWalls()
^
planning/behavior_velocity_no_stopping_area_module/src/manager.cpp:54:0: style: The function 'launchNewModules' is never used. [unusedFunction]
void NoStoppingAreaModuleManager::launchNewModules(
^
planning/behavior_velocity_no_stopping_area_module/src/manager.cpp:77:0: style: The function 'getModuleExpiredFunction' is never used. [unusedFunction]
NoStoppingAreaModuleManager::getModuleExpiredFunction(
^
planning/behavior_velocity_no_stopping_area_module/src/scene_no_stopping_area.cpp:108:0: style: The function 'modifyPathVelocity' is never used. [unusedFunction]
bool NoStoppingAreaModule::modifyPathVelocity(PathWithLaneId * path, StopReason * stop_reason)
^

Tests performed

No

Effects on system behavior

Interface changes

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

I've confirmed the contribution guidelines.
The PR follows the pull request guidelines.

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

The PR follows the pull request guidelines.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

There are no open discussions or they are tracked via tickets.

After all checkboxes are checked, anyone who has write access can merge the PR.

Signed-off-by: Ryuta Kambe <[email protected]>

shmpwk · 2024-05-13T00:37:08Z

@veqcc
Thank you for your OSS contribution in holiday!
Regarding your question about unused functions in debug.cpp, I agree to delete those functions. But let me make sure to ask @kosuke55 -san who seems to be a heavy user of no stopping area as a goal planner developper.

Other fixes looks goooooood to me!

kosuke55 · 2024-05-13T02:56:23Z

@veqcc

I have one question: there seems to be several unused functions in debug.cpp.

thanks! Is it ok to check only debug.cpp?
I think manager.cpp's functions (launchNewModules etc) are used 🤔

And other modules in behavior veclocity planner has same structure and fuctions. Do you have any idea why only no_stopping_area module got the unusedFunction error?

veqcc · 2024-05-13T05:37:42Z

@kosuke55

thanks! Is it ok to check only debug.cpp?
I think manager.cpp's functions (launchNewModules etc) are used 🤔

You are right 👍

And other modules in behavior veclocity planner has same structure and fuctions. Do you have any idea why only no_stopping_area module got the unusedFunction error?

Actualy, there are similar unusedFunction warnings in other modules.
Since this is a PR for behavior_velocity_no_stopping_area_module, I mentioned only related warnings.
If you prefer not to delete these functions, I think it also makes sense.

kosuke55 · 2024-05-13T05:50:52Z

@veqcc
It seem to get this warning about functions that is overridden. And I checked function in debug.cpp is also used. So we need to keep those functions.

kosuke55

LGTM!

veqcc · 2024-05-13T10:46:58Z

I will fix the build warning later

veqcc · 2024-05-14T00:47:59Z

@kosuke55
Hi, I have one question on this commit:
9e76436

Here, I have deleted dist_from_area_sum variable since it is only update inside the first if branch but soon breaks the loop.
Consequently, the parqameter margin is not necessary anymore, and in tern the following two function calls become totally the same https://github.com/autowarefoundation/autoware.universe/blob/b6bd75073a61757b6212c578409363680df2ddd8/planning/behavior_velocity_no_stopping_area_module/src/scene_no_stopping_area.cpp#L153C3-L160C43 .
Is it correct?

const Polygon2d stuck_vehicle_detect_area = generateEgoNoStoppingAreaLanePolygon(
    *path, current_pose->pose, ego_space_in_front_of_stuck_vehicle,
    planner_param_.detection_area_length);
const Polygon2d stop_line_detect_area = generateEgoNoStoppingAreaLanePolygon(
    *path, current_pose->pose, ego_space_in_front_of_stop_line,
    planner_param_.detection_area_length);
↓
const Polygon2d stuck_vehicle_detect_area = generateEgoNoStoppingAreaLanePolygon(
    *path, current_pose->pose, // margin is deleted
    planner_param_.detection_area_length);
const Polygon2d stop_line_detect_area = generateEgoNoStoppingAreaLanePolygon(
    *path, current_pose->pose, // margin is deleted
    planner_param_.detection_area_length);

planning/behavior_velocity_no_stopping_area_module/src/scene_no_stopping_area.cpp

kosuke55 · 2024-05-20T06:12:11Z

@veqcc

Here, I have deleted dist_from_area_sum variable since it is only update inside the first if branch but soon breaks the loop.
Consequently, the parqameter margin is not necessary anymore, and in tern the following two function calls become totally the same https://github.com/autowarefoundation/autoware.universe/blob/b6bd75073a61757b6212c578409363680df2ddd8/planning/behavior_velocity_no_stopping_area_module/src/scene_no_stopping_area.cpp#L153C3-L160C43 .
Is it correct?

The change was made in the PR #6517 here and I checked with @kaigohirao san. dist_from_area_sum was already no longer needed, so you can delete them.

veqcc · 2024-05-21T02:37:24Z

@kosuke55
Thank you for your detailed investigation!!
I understand we can delete dist_from_area_sum from generateEgoNoStoppingAreaLanePolygon function!!

Now, my question is this:
The generateEgoNoStoppingAreaLanePolygon function does not need margin parameter anymore, and thus the following two variables stuck_vehicle_detect_area and stop_line_detect_area will become exactly the same. It is true?

  const Polygon2d stuck_vehicle_detect_area = generateEgoNoStoppingAreaLanePolygon(
    *path, current_pose->pose, ego_space_in_front_of_stuck_vehicle,
    planner_param_.detection_area_length);
  const Polygon2d stop_line_detect_area = generateEgoNoStoppingAreaLanePolygon(
    *path, current_pose->pose, ego_space_in_front_of_stop_line,
    planner_param_.detection_area_length);
``
https://github.com/autowarefoundation/autoware.universe/blob/b6bd75073a61757b6212c578409363680df2ddd8/planning/behavior_velocity_no_stopping_area_module/src/scene_no_stopping_area.cpp#L153C3-L160C43

veqcc · 2024-05-21T05:01:58Z

@kosuke55
I have reverted the commit about dist_from_area_sum and created a new issue about it #7081 👍

kosuke55

@veqcc
We will work on the issue in other PR.
LGTM for your PR. Thank you!

autowarefoundation#6986) * remove redundant if branch Signed-off-by: Ryuta Kambe <[email protected]> * remove unused variable Signed-off-by: Ryuta Kambe <[email protected]> * remove redundant initialization Signed-off-by: Ryuta Kambe <[email protected]> --------- Signed-off-by: Ryuta Kambe <[email protected]>

veqcc added 3 commits May 12, 2024 13:20

remove redundant if branch

e0e49be

Signed-off-by: Ryuta Kambe <[email protected]>

remove unused variable

021129a

Signed-off-by: Ryuta Kambe <[email protected]>

remove redundant initialization

0eb34eb

Signed-off-by: Ryuta Kambe <[email protected]>

veqcc requested review from kosuke55, shmpwk and tkimura4 as code owners May 12, 2024 04:48

github-actions bot added the component:planning Route planning, decision-making, and navigation. (auto-assigned) label May 12, 2024

kosuke55 approved these changes May 13, 2024

View reviewed changes

kosuke55 added the run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label May 13, 2024

soblin requested changes May 17, 2024

View reviewed changes

planning/behavior_velocity_no_stopping_area_module/src/scene_no_stopping_area.cpp Show resolved Hide resolved

veqcc force-pushed the refactor/behavior_velocity_no_stopping_area_module branch from 2e41fe1 to 9e76436 Compare May 17, 2024 07:56

veqcc force-pushed the refactor/behavior_velocity_no_stopping_area_module branch from 9e76436 to 0eb34eb Compare May 21, 2024 04:51

veqcc mentioned this pull request May 21, 2024

Incorrect implimentation in planning/behavior_velocity_no_stopping_area_module #7081

Closed

3 tasks

kosuke55 approved these changes May 21, 2024

View reviewed changes

soblin approved these changes May 21, 2024

View reviewed changes

veqcc merged commit 982b5c9 into autowarefoundation:main May 21, 2024
20 of 22 checks passed

veqcc deleted the refactor/behavior_velocity_no_stopping_area_module branch May 21, 2024 06:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(behavior_velocity_no_stopping_area_module): cppcheck warnings #6986

refactor(behavior_velocity_no_stopping_area_module): cppcheck warnings #6986

veqcc commented May 12, 2024 •

edited

Loading

shmpwk commented May 13, 2024 •

edited

Loading

kosuke55 commented May 13, 2024 •

edited

Loading

veqcc commented May 13, 2024

kosuke55 commented May 13, 2024

kosuke55 left a comment

veqcc commented May 13, 2024

veqcc commented May 14, 2024

kosuke55 commented May 20, 2024

veqcc commented May 21, 2024

veqcc commented May 21, 2024

kosuke55 left a comment

refactor(behavior_velocity_no_stopping_area_module): cppcheck warnings #6986

refactor(behavior_velocity_no_stopping_area_module): cppcheck warnings #6986

Conversation

veqcc commented May 12, 2024 • edited Loading

Description

Question

Tests performed

Effects on system behavior

Interface changes

Pre-review checklist for the PR author

In-review checklist for the PR reviewers

Post-review checklist for the PR author

shmpwk commented May 13, 2024 • edited Loading

kosuke55 commented May 13, 2024 • edited Loading

veqcc commented May 13, 2024

kosuke55 commented May 13, 2024

kosuke55 left a comment

Choose a reason for hiding this comment

veqcc commented May 13, 2024

veqcc commented May 14, 2024

kosuke55 commented May 20, 2024

veqcc commented May 21, 2024

veqcc commented May 21, 2024

kosuke55 left a comment

Choose a reason for hiding this comment

veqcc commented May 12, 2024 •

edited

Loading

shmpwk commented May 13, 2024 •

edited

Loading

kosuke55 commented May 13, 2024 •

edited

Loading