Skip to content

Improve the function extract_type_identifier #2923

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Aug 18, 2025
Merged

Improve the function extract_type_identifier #2923

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
08006bd
Improve the function extract_type_identifier
Barry-Xu-2018 Aug 4, 2025
629af57
Improve code execution efficiency based on review comments
Barry-Xu-2018 Aug 5, 2025
db838fc
Add the missing header file
Barry-Xu-2018 Aug 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion rclcpp/src/rclcpp/typesupport_helpers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

#include "rclcpp/typesupport_helpers.hpp"

#include <algorithm>
#include <functional>
#include <memory>
#include <sstream>
Expand Down Expand Up @@ -73,6 +74,21 @@ const void * get_typesupport_handle_impl(
}
}

// Trim leading and trailing whitespace from the string.
std::string string_trim(std::string_view str_v)
Copy link
Collaborator

Choose a reason for hiding this comment

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

how about the following? instead of removing the space one by one? i believe it only scans the string twice, not once per whitespace character, and could be more readable and idiomatic.

std::string string_trim(std::string_view str_v) {
  auto begin = std::find_if_not(str_v.begin(), str_v.end(), [](unsigned char ch) {
    return std::isspace(ch);
  });
  auto end = std::find_if_not(str_v.rbegin(), str_v.rend(), [](unsigned char ch) {
    return std::isspace(ch);
  }).base();
  if (begin >= end) return {};
  return std::string(begin, end);
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks. This way is more efficient.

{
auto begin = std::find_if_not(str_v.begin(), str_v.end(), [](unsigned char ch) {
Copy link
Contributor

Choose a reason for hiding this comment

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

include <algorithm>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you

return std::isspace(ch);
});
auto end = std::find_if_not(str_v.rbegin(), str_v.rend(), [](unsigned char ch) {
return std::isspace(ch);
}).base();
if (begin >= end) {
return {};
}
return std::string(begin, end);
}

} // anonymous namespace

std::tuple<std::string, std::string, std::string>
Expand All @@ -82,6 +98,7 @@ extract_type_identifier(const std::string & full_type)
auto sep_position_back = full_type.find_last_of(type_separator);
auto sep_position_front = full_type.find_first_of(type_separator);
if (sep_position_back == std::string::npos ||
sep_position_front == 0 ||
sep_position_back == 0 ||
sep_position_back == full_type.length() - 1)
{
Expand All @@ -97,7 +114,8 @@ extract_type_identifier(const std::string & full_type)
}
std::string type_name = full_type.substr(sep_position_back + 1);

return std::make_tuple(package_name, middle_module, type_name);
return std::make_tuple(
string_trim(package_name), string_trim(middle_module), string_trim(type_name));
}

std::string get_typesupport_library_path(
Expand Down
12 changes: 12 additions & 0 deletions rclcpp/test/rclcpp/test_typesupport_helpers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,3 +158,15 @@ TEST(TypesupportHelpersTest, test_throw_exception_with_invalid_type) {
rclcpp::get_action_typesupport_handle(invalid_type, "rosidl_typesupport_cpp", *library),
std::runtime_error);
}

TEST(TypesupportHelpersTest, throws_exception_if_filetype_has_multiple_slashes_at_start) {
EXPECT_ANY_THROW(rclcpp::extract_type_identifier("//name_with_slashes_at_start"));
}

TEST(TypesupportHelpersTestV3, ProcessesValidTypeWithWhitespace) {
std::string package, middle, name;
std::tie(package, middle, name) = rclcpp::extract_type_identifier(" package/ name ");
EXPECT_EQ(package, "package");
EXPECT_TRUE(middle.empty());
EXPECT_EQ(name, "name");
}