Skip to content

Commit

Permalink
Make sure to check for empty extension before accessing front. (#1764)
Browse files Browse the repository at this point in the history
The documentation for std::string::front() says that calling it
on an empty string is undefined behavior.  It actually seems to
work on all platforms except Windows Debug, where it throws an
error.  Make sure to check for empty first.

We also notice that there is no reason to check the
existing_sub_namespace for empty; the rest of the code handles
that fine.  So remove that check.

Finally, we add an anonymous namespace so that these functions do
not pollute the global namespace.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
  • Loading branch information
clalancette committed Sep 3, 2021
1 parent ecb81ef commit 665e377
Showing 1 changed file with 11 additions and 6 deletions.
17 changes: 11 additions & 6 deletions rclcpp/src/rclcpp/node.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -47,25 +47,28 @@ using rclcpp::Node;
using rclcpp::NodeOptions;
using rclcpp::exceptions::throw_from_rcl_error;

namespace
{

RCLCPP_LOCAL
std::string
extend_sub_namespace(const std::string & existing_sub_namespace, const std::string & extension)
{
// Assumption is that the existing_sub_namespace does not need checking
// because it would be checked already when it was set with this function.

// check if the new sub-namespace extension is absolute
if (extension.front() == '/') {
if (extension.empty()) {
throw rclcpp::exceptions::NameValidationError(
"sub_namespace",
extension.c_str(),
"a sub-namespace should not have a leading /",
"sub-nodes should not extend nodes by an empty sub-namespace",
0);
} else if (existing_sub_namespace.empty() && extension.empty()) {
} else if (extension.front() == '/') {
// check if the new sub-namespace extension is absolute
throw rclcpp::exceptions::NameValidationError(
"sub_namespace",
extension.c_str(),
"sub-nodes should not extend nodes by an empty sub-namespace",
"a sub-namespace should not have a leading /",
0);
}

Expand All @@ -76,7 +79,7 @@ extend_sub_namespace(const std::string & existing_sub_namespace, const std::stri
new_sub_namespace = existing_sub_namespace + "/" + extension;
}

// remove any trailing `/` so that new extensions do no result in `//`
// remove any trailing `/` so that new extensions do not result in `//`
if (new_sub_namespace.back() == '/') {
new_sub_namespace = new_sub_namespace.substr(0, new_sub_namespace.size() - 1);
}
Expand Down Expand Up @@ -104,6 +107,8 @@ create_effective_namespace(const std::string & node_namespace, const std::string
}
}

} // namespace

Node::Node(
const std::string & node_name,
const NodeOptions & options)
Expand Down

0 comments on commit 665e377

Please sign in to comment.