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.

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

Implement rmw_set_log_severity #149

Merged
merged 1 commit into from
Apr 13, 2020

Conversation

rotu
Copy link
Collaborator

@rotu rotu commented Apr 10, 2020

The existing rmw_set_log_severity currently just throws an error. This implements it.

@rotu rotu mentioned this pull request Apr 10, 2020
@dirk-thomas
Copy link
Member

@rotu Please run CI builds showing that this patch fixes the test failures #147 was addressing.

@rotu
Copy link
Collaborator Author

rotu commented Apr 11, 2020

This on its own does not fix the test failures #147 was addressing. It may or may not be possible to fix the test using this development, depending on the design of launch_testing.

@dirk-thomas
Copy link
Member

Please either come up with a working solution to resolve the test failures or we should reopen the previous PR and merge it as a temporary solution.

@rotu
Copy link
Collaborator Author

rotu commented Apr 11, 2020

@dirk-thomas a working solution is to remove the strict flag in the calls to expect_output. There is no guarantee that this is the only output from Cyclone - and putting human-readable warning messages on stderr is a feature, not a bug.

@dirk-thomas
Copy link
Member

dirk-thomas commented Apr 11, 2020

a working solution is to remove the strict flag in the calls to expect_output.

These CLI tests checks for strict output for a reason. They not only ensure that certain output is present but also ensure that no other output like warnings / errors / wrong information is present. So no, strict=False is not an appropriate solution.

Please either suggest a viable alternative or we should use the previous proposed PR.

@rotu
Copy link
Collaborator Author

rotu commented Apr 11, 2020

@dirk-thomas This discussion does not belong on this PR. Creating an issue and moving discussion there.

@rotu rotu requested a review from hidmic April 11, 2020 06:22
@rotu rotu merged commit 5781044 into ros2:master Apr 13, 2020
@Karsten1987
Copy link
Contributor

I think this change introduced a warning implicit-fallthrough. Is that on purpose? If so, does it make sense to explicitly disable this warning around that block of code? That would help people like me how compile their workspace with -Werror ;-)

--- stderr: rmw_cyclonedds_cpp
/home/karsten/ros2_ws/src/ros2/rmw_cyclonedds/rmw_cyclonedds_cpp/src/rmw_node.cpp: In function ‘rmw_ret_t rmw_set_log_severity(rmw_log_severity_t)’:
/home/karsten/ros2_ws/src/ros2/rmw_cyclonedds/rmw_cyclonedds_cpp/src/rmw_node.cpp:356:12: error: this statement may fall through [-Werror=implicit-fallthrough=]
       mask |= DDS_LC_DISCOVERY | DDS_LC_THROTTLE | DDS_LC_CONFIG;
            ^
/home/karsten/ros2_ws/src/ros2/rmw_cyclonedds/rmw_cyclonedds_cpp/src/rmw_node.cpp:357:5: note: here
     case RMW_LOG_SEVERITY_INFO:
     ^~~~
/home/karsten/ros2_ws/src/ros2/rmw_cyclonedds/rmw_cyclonedds_cpp/src/rmw_node.cpp:358:12: error: this statement may fall through [-Werror=implicit-fallthrough=]
       mask |= DDS_LC_INFO;
            ^
/home/karsten/ros2_ws/src/ros2/rmw_cyclonedds/rmw_cyclonedds_cpp/src/rmw_node.cpp:359:5: note: here
     case RMW_LOG_SEVERITY_WARN:
     ^~~~
/home/karsten/ros2_ws/src/ros2/rmw_cyclonedds/rmw_cyclonedds_cpp/src/rmw_node.cpp:360:12: error: this statement may fall through [-Werror=implicit-fallthrough=]
       mask |= DDS_LC_WARNING;
            ^
/home/karsten/ros2_ws/src/ros2/rmw_cyclonedds/rmw_cyclonedds_cpp/src/rmw_node.cpp:361:5: note: here
     case RMW_LOG_SEVERITY_ERROR:
     ^~~~
/home/karsten/ros2_ws/src/ros2/rmw_cyclonedds/rmw_cyclonedds_cpp/src/rmw_node.cpp:362:12: error: this statement may fall through [-Werror=implicit-fallthrough=]
       mask |= DDS_LC_ERROR;
            ^
/home/karsten/ros2_ws/src/ros2/rmw_cyclonedds/rmw_cyclonedds_cpp/src/rmw_node.cpp:363:5: note: here
     case RMW_LOG_SEVERITY_FATAL:
     ^~~~
cc1plus: all warnings being treated as errors

@rotu
Copy link
Collaborator Author

rotu commented Apr 14, 2020

@Karsten1987

I think this change introduced a warning implicit-fallthrough. Is that on purpose?

The fallthrough is intentional, and I should fix this warning. What compiler and build flags are you using?

@Karsten1987
Copy link
Contributor

using g++ 7.4.0 on ubuntu 18.04 with -Werror. I don't see that warning with Clang (clang-1001.0.46.4)

@rotu rotu deleted the rmw_log_severity branch April 14, 2020 05:50
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