Skip to content

compare the return value to constants #665

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

Closed
iuhilnehc-ynos wants to merge 2 commits into from
Closed

compare the return value to constants #665

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
37c7657
compare the return value to constants
May 28, 2020
600edd2
keep the original logic
May 28, 2020
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
12 changes: 4 additions & 8 deletions rcl/src/rcl/logging.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,15 +88,11 @@ rcl_logging_configure_with_output_handler(
if (g_rcl_logging_ext_lib_enabled) {
status = rcl_logging_external_initialize(config_file, g_logging_allocator);
if (RCL_RET_OK == status) {
// TODO(dirk-thomas) the return value should be typed and compared to
// constants instead of zero
int logging_status = rcl_logging_external_set_logger_level(
NULL, default_level);
if (logging_status != 0) {
status = RCL_RET_ERROR;
status = rcl_logging_external_set_logger_level(NULL, default_level);
if (RCL_RET_OK == status) {

@fujitatomoya fujitatomoya May 28, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This changes a logic, only when log level can be set, then register handler?
Previously registering handler anyway even if log level cannot be set.

@iuhilnehc-ynos iuhilnehc-ynos May 28, 2020 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes. And I think this is the correct logic. Maybe I was wrong.

@iuhilnehc-ynos iuhilnehc-ynos May 28, 2020

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@fujitatomoya
After consideration, I shouldn't update the original logic while refactoring the source code.
I will update it.

@fujitatomoya fujitatomoya May 28, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@iuhilnehc-ynos

I guess that original logic is meant to, I'd like to hear from others.

g_rcl_logging_out_handlers[g_rcl_logging_num_out_handlers++] =
rcl_logging_ext_lib_output_handler;
}
g_rcl_logging_out_handlers[g_rcl_logging_num_out_handlers++] =
rcl_logging_ext_lib_output_handler;
}
}
rcutils_logging_set_output_handler(output_handler);
Expand Down