Skip to content

Redundant .c_str() usage in rclcpp_components triggers ament_clang_tidy warning#2718

Merged
fujitatomoya merged 2 commits intoros2:humblefrom
LihanChen2004:fix-redundant-c_str
Dec 29, 2024
Merged

Redundant .c_str() usage in rclcpp_components triggers ament_clang_tidy warning#2718
fujitatomoya merged 2 commits intoros2:humblefrom
LihanChen2004:fix-redundant-c_str

Conversation

@LihanChen2004
Copy link
Copy Markdown

@LihanChen2004 LihanChen2004 commented Dec 28, 2024

The code snippet:

std::string name = clazz.c_str();

is redundant and unnecessary when initializing a std::string object.

The method std::string::operator=(const char*), which takes a C-style string (const char*), introduces an extra conversion step when the input is already a std::string. Specifically, the c_str() method is used to extract a const char* pointer from clazz, and then std::string reconstructs the string from the pointer.

Instead, the simpler and more efficient approach is:

std::string name = clazz;

This directly utilizes the copy constructor or move constructor of std::string, avoiding the unnecessary intermediate conversion to const char*. It results in cleaner, more idiomatic C++ code with no performance penalty.

Addresses: #2717

Signed-off-by: LihanChen2004 <757003373@qq.com>
LihanChen2004 added a commit to SMBU-PolarBear-Robotics-Team/small_gicp_relocalization that referenced this pull request Dec 28, 2024
NOTE: `ament_cmake_clang_tidy` will be added when the PR will be merged.
ros2/rclcpp#2718
LihanChen2004 added a commit to SMBU-PolarBear-Robotics-Team/pb2025_sentry_nav that referenced this pull request Dec 28, 2024
NOTE: `ament_cmake_clang_tidy` will be added when the PR will be merged.
ros2/rclcpp#2718
Copy link
Copy Markdown
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LihanChen2004 thanks for creating PR.

Co-authored-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
Signed-off-by: LihanChen2004 <74599182+LihanChen2004@users.noreply.github.com>
@clalancette
Copy link
Copy Markdown
Contributor

Pulls: #2718
Gist: https://gist.githubusercontent.com/clalancette/db85df4ed40b52a0cb3945c91d45b99b/raw/92681d90a6dba181cd4c37559e74d034beb99aa0/ros2.repos
BUILD args: --packages-above-and-dependencies rclcpp_components
TEST args: --packages-above rclcpp_components
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15015

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

Copy link
Copy Markdown
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, CI failing because this targets to humble only. i will restart.

@fujitatomoya
Copy link
Copy Markdown
Collaborator

Pulls: #2718
Gist: https://gist.githubusercontent.com/fujitatomoya/3d68220de5abb05de1c42d5264ce9e62/raw/fcdee4cfd58f36c0f79f451e2de5284503621470/ros2.repos
BUILD args: --packages-above-and-dependencies rclcpp_components
TEST args: --packages-above rclcpp_components
ROS Distro: humble
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15016

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants