Skip to content
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

Add few warning compiler options to error (backport #1181) #1816

Merged
merged 2 commits into from
Oct 27, 2024
Merged

Add few warning compiler options to error (backport #1181) #1816

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6ac94c5
Add few warning compiler options to error (#1181)
saikishor Nov 27, 2023
bb6e50f
Deactivate compiler warning for class_loader
christophfroehlich Oct 26, 2024
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
2 changes: 1 addition & 1 deletion controller_interface/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ cmake_minimum_required(VERSION 3.5)
project(controller_interface)

if(CMAKE_COMPILER_IS_GNUCXX OR CMAKE_CXX_COMPILER_ID MATCHES "Clang")
add_compile_options(-Wall -Wextra -Wconversion)
add_compile_options(-Wall -Wextra -Werror=conversion -Werror=unused-but-set-variable -Werror=return-type -Werror=shadow)
endif()

find_package(ament_cmake REQUIRED)
Expand Down
2 changes: 1 addition & 1 deletion controller_manager/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ cmake_minimum_required(VERSION 3.5)
project(controller_manager)

if(CMAKE_COMPILER_IS_GNUCXX OR CMAKE_CXX_COMPILER_ID MATCHES "Clang")
add_compile_options(-Wall -Wextra -Wconversion)
add_compile_options(-Wall -Wextra -Werror=conversion -Werror=unused-but-set-variable -Werror=return-type -Werror=shadow)
endif()

set(THIS_PACKAGE_INCLUDE_DEPENDS
Expand Down
48 changes: 23 additions & 25 deletions controller_manager/src/controller_manager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,15 +444,15 @@ void ControllerManager::init_resource_manager(const std::string & robot_descript
// BEGIN: Keep old functionality on for backwards compatibility
std::vector<std::string> activate_components_on_start = std::vector<std::string>({});
get_parameter("activate_components_on_start", activate_components_on_start);
rclcpp_lifecycle::State active_state(
State::PRIMARY_STATE_ACTIVE, hardware_interface::lifecycle_state_names::ACTIVE);
if (!activate_components_on_start.empty())
{
RCLCPP_WARN(
get_logger(),
"[Deprecated]: Parameter 'activate_components_on_start' is deprecated. "
"Components are activated per default. Don't use this parameters in combination with the new "
"'hardware_components_initial_state' parameter structure.");
rclcpp_lifecycle::State active_state(
State::PRIMARY_STATE_ACTIVE, hardware_interface::lifecycle_state_names::ACTIVE);
for (const auto & component : activate_components_on_start)
{
resource_manager_->set_component_state(component, active_state);
Expand All @@ -464,8 +464,6 @@ void ControllerManager::init_resource_manager(const std::string & robot_descript
// activate all other components
for (const auto & [component, state] : components_to_activate)
{
rclcpp_lifecycle::State active_state(
State::PRIMARY_STATE_ACTIVE, hardware_interface::lifecycle_state_names::ACTIVE);
if (
resource_manager_->set_component_state(component, active_state) ==
hardware_interface::return_type::ERROR)
Expand Down Expand Up @@ -988,7 +986,7 @@ controller_interface::return_type ControllerManager::switch_controller(
auto controller_it = std::find_if(
controllers.begin(), controllers.end(),
std::bind(controller_name_compare, std::placeholders::_1, *ctrl_it));
controller_interface::return_type ret = controller_interface::return_type::OK;
controller_interface::return_type status = controller_interface::return_type::OK;

// if controller is not inactive then do not do any following-controllers checks
if (!is_controller_inactive(controller_it->c))
Expand All @@ -998,14 +996,14 @@ controller_interface::return_type ControllerManager::switch_controller(
"Controller with name '%s' is not inactive so its following "
"controllers do not have to be checked, because it cannot be activated.",
controller_it->info.name.c_str());
ret = controller_interface::return_type::ERROR;
status = controller_interface::return_type::ERROR;
}
else
{
ret = check_following_controllers_for_activate(controllers, strictness, controller_it);
status = check_following_controllers_for_activate(controllers, strictness, controller_it);
}

if (ret != controller_interface::return_type::OK)
if (status != controller_interface::return_type::OK)
{
RCLCPP_WARN(
get_logger(),
Expand Down Expand Up @@ -1039,22 +1037,22 @@ controller_interface::return_type ControllerManager::switch_controller(
auto controller_it = std::find_if(
controllers.begin(), controllers.end(),
std::bind(controller_name_compare, std::placeholders::_1, *ctrl_it));
controller_interface::return_type ret = controller_interface::return_type::OK;
controller_interface::return_type status = controller_interface::return_type::OK;

// if controller is not active then skip preceding-controllers checks
if (!is_controller_active(controller_it->c))
{
RCLCPP_WARN(
get_logger(), "Controller with name '%s' can not be deactivated since it is not active.",
controller_it->info.name.c_str());
ret = controller_interface::return_type::ERROR;
status = controller_interface::return_type::ERROR;
}
else
{
ret = check_preceeding_controllers_for_deactivate(controllers, strictness, controller_it);
status = check_preceeding_controllers_for_deactivate(controllers, strictness, controller_it);
}

if (ret != controller_interface::return_type::OK)
if (status != controller_interface::return_type::OK)
{
RCLCPP_WARN(
get_logger(),
Expand Down Expand Up @@ -1135,11 +1133,11 @@ controller_interface::return_type ControllerManager::switch_controller(
// check for double stop
if (!is_active && in_deactivate_list)
{
auto ret = handle_conflict(
auto conflict_status = handle_conflict(
"Could not deactivate controller '" + controller.info.name + "' since it is not active");
if (ret != controller_interface::return_type::OK)
if (conflict_status != controller_interface::return_type::OK)
{
return ret;
return conflict_status;
}
in_deactivate_list = false;
deactivate_request_.erase(deactivate_list_it);
Expand All @@ -1148,11 +1146,11 @@ controller_interface::return_type ControllerManager::switch_controller(
// check for doubled activation
if (is_active && !in_deactivate_list && in_activate_list)
{
auto ret = handle_conflict(
auto conflict_status = handle_conflict(
"Could not activate controller '" + controller.info.name + "' since it is already active");
if (ret != controller_interface::return_type::OK)
if (conflict_status != controller_interface::return_type::OK)
{
return ret;
return conflict_status;
}
in_activate_list = false;
activate_request_.erase(activate_list_it);
Expand All @@ -1161,21 +1159,21 @@ controller_interface::return_type ControllerManager::switch_controller(
// check for illegal activation of an unconfigured/finalized controller
if (!is_inactive && !in_deactivate_list && in_activate_list)
{
auto ret = handle_conflict(
auto conflict_status = handle_conflict(
"Could not activate controller '" + controller.info.name +
"' since it is not in inactive state");
if (ret != controller_interface::return_type::OK)
if (conflict_status != controller_interface::return_type::OK)
{
return ret;
return conflict_status;
}
in_activate_list = false;
activate_request_.erase(activate_list_it);
}

const auto extract_interfaces_for_controller =
[this](const ControllerSpec controller, std::vector<std::string> & request_interface_list)
[this](const ControllerSpec ctrl, std::vector<std::string> & request_interface_list)
{
auto command_interface_config = controller.c->command_interface_configuration();
auto command_interface_config = ctrl.c->command_interface_configuration();
std::vector<std::string> command_interface_names = {};
if (command_interface_config.type == controller_interface::interface_configuration_type::ALL)
{
Expand Down Expand Up @@ -1775,8 +1773,8 @@ void ControllerManager::reload_controller_libraries_service_cb(
loaded_controllers = get_controller_names();
{
// lock controllers
std::lock_guard<std::recursive_mutex> guard(rt_controllers_wrapper_.controllers_lock_);
for (const auto & controller : rt_controllers_wrapper_.get_updated_list(guard))
std::lock_guard<std::recursive_mutex> ctrl_guard(rt_controllers_wrapper_.controllers_lock_);
for (const auto & controller : rt_controllers_wrapper_.get_updated_list(ctrl_guard))
{
if (is_controller_active(*controller.c))
{
Expand Down
2 changes: 1 addition & 1 deletion hardware_interface/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ cmake_minimum_required(VERSION 3.5)
project(hardware_interface)

if(CMAKE_COMPILER_IS_GNUCXX OR CMAKE_CXX_COMPILER_ID MATCHES "Clang")
add_compile_options(-Wall -Wextra -Wconversion)
add_compile_options(-Wall -Wextra -Werror=conversion -Werror=unused-but-set-variable -Werror=return-type -Werror=shadow)
endif()

set(THIS_PACKAGE_INCLUDE_DEPENDS
Expand Down
8 changes: 4 additions & 4 deletions hardware_interface/src/mock_components/generic_system.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -594,12 +594,12 @@ return_type GenericSystem::read(const rclcpp::Time & /*time*/, const rclcpp::Dur
}
else
{
for (size_t j = 0; j < joint_states_[POSITION_INTERFACE_INDEX].size(); ++j)
for (size_t k = 0; k < joint_states_[POSITION_INTERFACE_INDEX].size(); ++k)
{
if (!std::isnan(joint_commands_[POSITION_INTERFACE_INDEX][j]))
if (!std::isnan(joint_commands_[POSITION_INTERFACE_INDEX][k]))
{
joint_states_[POSITION_INTERFACE_INDEX][j] = // apply offset to positions only
joint_commands_[POSITION_INTERFACE_INDEX][j] +
joint_states_[POSITION_INTERFACE_INDEX][k] = // apply offset to positions only
joint_commands_[POSITION_INTERFACE_INDEX][k] +
(custom_interface_with_following_offset_.empty() ? position_state_following_offset_
: 0.0);
}
Expand Down
24 changes: 13 additions & 11 deletions hardware_interface_testing/test/test_resource_manager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,21 +232,23 @@ TEST_F(ResourceManagerTest, resource_claiming)
// Activate components to get all interfaces available
activate_components(rm);

const auto command_interface = "joint1/position";
EXPECT_TRUE(rm.command_interface_is_available(command_interface));
EXPECT_FALSE(rm.command_interface_is_claimed(command_interface));

{
auto position_command_interface = rm.claim_command_interface(command_interface);
EXPECT_TRUE(rm.command_interface_is_available(command_interface));
EXPECT_TRUE(rm.command_interface_is_claimed(command_interface));
const auto key = "joint1/position";
EXPECT_TRUE(rm.command_interface_is_available(key));
EXPECT_FALSE(rm.command_interface_is_claimed(key));

{
EXPECT_ANY_THROW(rm.claim_command_interface(command_interface));
EXPECT_TRUE(rm.command_interface_is_available(command_interface));
auto position_command_interface = rm.claim_command_interface(key);
EXPECT_TRUE(rm.command_interface_is_available(key));
EXPECT_TRUE(rm.command_interface_is_claimed(key));
{
EXPECT_ANY_THROW(rm.claim_command_interface(key));
EXPECT_TRUE(rm.command_interface_is_available(key));
}
}
EXPECT_TRUE(rm.command_interface_is_available(key));
EXPECT_FALSE(rm.command_interface_is_claimed(key));
}
EXPECT_TRUE(rm.command_interface_is_available(command_interface));
EXPECT_FALSE(rm.command_interface_is_claimed(command_interface));

// command interfaces can only be claimed once
for (const auto & interface_key :
Expand Down
3 changes: 3 additions & 0 deletions joint_limits/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ project(joint_limits)
if(NOT CMAKE_CXX_STANDARD)
set(CMAKE_CXX_STANDARD 14)
endif()
if(CMAKE_CXX_COMPILER_ID MATCHES "(GNU|Clang)")
add_compile_options(-Wall -Wextra -Werror=conversion -Werror=unused-but-set-variable -Werror=return-type -Werror=shadow)
endif()

if(CMAKE_COMPILER_IS_GNUCXX OR CMAKE_CXX_COMPILER_ID MATCHES "Clang")
add_compile_options(-Wall -Wextra -Wconversion)
Expand Down
5 changes: 4 additions & 1 deletion transmission_interface/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@ cmake_minimum_required(VERSION 3.5)
project(transmission_interface)

if(CMAKE_COMPILER_IS_GNUCXX OR CMAKE_CXX_COMPILER_ID MATCHES "Clang")
add_compile_options(-Wall -Wextra -Wpedantic -Wconversion)
add_compile_options(-Wall -Wextra -Werror=conversion -Werror=unused-but-set-variable -Werror=return-type
# deactivating this, because of issue in upstream class_loader_imp.hpp
# -Werror=shadow
)
endif()

set(THIS_PACKAGE_INCLUDE_DEPENDS
Expand Down
Loading