Skip to content

Commit

Permalink
Avoid unused return value warning
Browse files Browse the repository at this point in the history
  • Loading branch information
Esteve Fernandez committed Dec 4, 2015
1 parent 13d36a1 commit e0f0819
Showing 1 changed file with 11 additions and 1 deletion.
12 changes: 11 additions & 1 deletion rclcpp/src/rclcpp/utilities.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,17 @@ rclcpp::utilities::init(int argc, char * argv[])
// NOLINTNEXTLINE(runtime/arrays)
char error_string[error_length];
#ifndef _WIN32
strerror_r(errno, error_string, error_length);
#if (_POSIX_C_SOURCE >= 200112L || _XOPEN_SOURCE >= 600) && ! _GNU_SOURCE
int error_status = strerror_r(errno, error_string, error_length);
if (error_status != 0) {
throw std::runtime_error("Failed to set error message");

This comment has been minimized.

Sign in to view

Copy link
@wjwwood

wjwwood Dec 4, 2015

Member

This isn't really setting the error message. Maybe something like "Failed to get error string for errno: " + std::to_string(errno) would be better?

This comment has been minimized.

Sign in to view

Copy link
@esteve

esteve Dec 4, 2015

Member

Fixed.
}
#else
char * msg = strerror_r(errno, error_string, error_length);
if (msg != error_string) {
strncpy(error_string, msg, sizeof(error_string));
}
#endif
#else
strerror_s(error_string, error_length, errno);
#endif
Expand Down

8 comments on commit e0f0819

@dirk-thomas
Copy link
Member

Choose a reason for hiding this comment

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

Can we not just use the code we had before the PR introducing the warning.

auto rc = strerror_r(errno, error_string, error_length);
if (rc) {
  throw std::runtime_error("Failed to set SIGINT signal handler: (" + std::to_string(errno) + ") unable to retrieve error string");
}

I don't think any of our currently supported platforms requires the platform checking logic proposed in this patch.

@wjwwood
Copy link
Member

@wjwwood wjwwood commented on e0f0819 Dec 4, 2015

Choose a reason for hiding this comment

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

I don't think so because on some systems strerror_r will return a string and leave the buffer unused (which in this case is error_string) and in that case we have to do the strncpy that @esteve added.

@dirk-thomas
Copy link
Member

Choose a reason for hiding this comment

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

Which of our platforms has which of the two (XSI-compliant vs. GNU-specific) behaviors?

@wjwwood
Copy link
Member

@wjwwood wjwwood commented on e0f0819 Dec 4, 2015

Choose a reason for hiding this comment

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

I don't know, but looking around on the internet it looks like Android might be different from Ubuntu (for example). Some things I found:

@dirk-thomas
Copy link
Member

Choose a reason for hiding this comment

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

Since we don't have a platform to test the proposed code I would choose to keep the code as simple as possible. Once we get to the point that we want to build on a platform which requires more we can update the code knowing that it will actually work. As your first link nicely shows: just adding the snippet without trying it on a real platform will very likely not work as expected.

@esteve
Copy link
Member

@esteve esteve commented on e0f0819 Dec 4, 2015

Choose a reason for hiding this comment

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

Apparently OSX requires the XSI version (albeit the ifdef logic doesn't seem to work):

http://ci.ros2.org/job/ci_osx/542

and Linux needs the GNU one, I'm going to keep this and apply @wjwwood's feedback and it should be ready for merging.

@wjwwood
Copy link
Member

@wjwwood wjwwood commented on e0f0819 Dec 4, 2015

Choose a reason for hiding this comment

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

OS X does use the XSI version. I just wrote a program to test it:

#include <string.h>
#include <stdio.h>

void check(int ret) { printf("XSI\n"); }
void check(char * ret) { printf("GNU\n"); }

int main() {
  char buf[1024];
  check(strerror_r(3, buf, 1024));
  return 0;
}

Outputs:

% ./junk
XSI

@esteve
Copy link
Member

@esteve esteve commented on e0f0819 Dec 4, 2015

Choose a reason for hiding this comment

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

CI jobs:

http://ci.ros2.org/job/ci_linux/660/
http://ci.ros2.org/job/ci_osx/543/
http://ci.ros2.org/job/ci_windows_opensplice/736/
http://ci.ros2.org/job/ci_windows_connext_dynamic/23/
http://ci.ros2.org/job/ci_windows_connext_static/22/

Please sign in to comment.