PR for issue#70 #98
PR for issue#70 #98
Conversation
|
Commenting to link to issue #70 |
src/logging.c
Outdated
| const char * line_buffered; | ||
| const char * ret_str = rcutils_get_env("RCUTILS_CONSOLE_STDOUT_LINE_BUFFERED", &line_buffered); | ||
|
|
||
| if (strcmp(line_buffered,"1")) { |
There was a problem hiding this comment.
Without checking ret_str for potential errors you can't access line_buffered. It might still be uninitialized.
See docblock.
src/logging.c
Outdated
| if (strcmp(line_buffered,"1")) { | ||
| g_force_stdout_line_buffered = true; | ||
| } | ||
| else if (NULL == ret_str || strcmp(output_format, "0") != 0) { |
There was a problem hiding this comment.
The check for NULL == ret_str seems also wrong here.
|
@dirk-thomas Here is the unit test results after the recent commits Label Time Summary: Total Test time (real) = 9.59 sec |
src/logging.c
Outdated
| } | ||
| } | ||
| } else { | ||
| if (NULL != ret_str) { |
There was a problem hiding this comment.
This condition is redundant with line 68. Please remove it from the patch.
src/logging.c
Outdated
| } | ||
| } else { | ||
| if (NULL != ret_str) { | ||
| fprintf(stderr, "Error getting env var: %s\n", ret_str); |
There was a problem hiding this comment.
Please add the name of the environment variable here so that the reader has a better idea where it is failing.
Also added the name of the environment variable failing in the same function block,
|
@dirk-thomas Thanks for the review. Made the changes as requested.Also performed the unit test locally. |
src/logging.c
Outdated
| } | ||
| } else { | ||
| fprintf(stderr, "Error getting env. variable" | ||
| "RCUTILS_CONSOLE_STDOUT_LINE_BUFFERED %s\n", ret_str); |
There was a problem hiding this comment.
There is currently no space between the two wrapped string.
A colon after RCUTILS_CONSOLE_STDOUT_LINE_BUFFERED before %s would be good too for the readability of the error message.
|
@dirk-thomas Oops.My apologies for overlooking that. |
src/logging.c
Outdated
| if (!strcmp(line_buffered, "0")) { | ||
| fprintf(stderr, | ||
| "Warning: unexpected value %s specified for RCUTILS_CONSOLE_STDOUT_LINE_BUFFERED." | ||
| "Default value 0 will be used. Valid values are 1 or 0.\n", |
There was a problem hiding this comment.
There is still no space between the two wrapped strings.
Apply space for wraped strings inside fprintf.
|
I just applied the missing space I was referring to. |
|
dirk-thomas
added
in review
|
@sagniknitr Thank you for contributing this option. |
|
No problem @dirk-thomas . Thank you for reviewing my code.I really look forward to more contributions. |
No description provided.