-
Notifications
You must be signed in to change notification settings - Fork 100
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
use strnlen_s instead of strnlen if available #430
base: rolling
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
basically i am not inclined to take this C non-standard for portability, but there are already __STDC_LIB_EXT1__
so that should not block this PR.
(note) in other way, how about adding memchr(string, '\0', max_string);
? and that can avoid calling strnlen_s
? so that we can remove __STDC_LIB_EXT1__
.
src/error_handling.c
Outdated
@@ -25,6 +25,7 @@ extern "C" | |||
#include <stdbool.h> | |||
#include <stdio.h> | |||
#include <stdlib.h> | |||
#define __STDC_WANT_LIB_EXT1__ 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this required to define here? it looks like rcutils/error_handling.h
already has defined that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this example says that the define
shall be before the #include <string.h>
. I'm not sure whether it is good to rely on what was defined in other headers.
#define __STDC_WANT_LIB_EXT1__ 1
#include <string.h>
#include <stdio.h>
int main(void)
{
const char str[] = "How many characters does this string contain?";
printf("without null character: %zu\n", strlen(str));
printf("with null character: %zu\n", sizeof str);
#ifdef __STDC_LIB_EXT1__
printf("without null character: %zu\n", strnlen_s(str, sizeof str));
#endif
}
src/error_handling_helpers.h
Outdated
#define __STDC_WANT_LIB_EXT1__ 1 | ||
#include <string.h> | ||
|
||
#include <rcutils/error_handling.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here? there is even #define __STDC_WANT_LIB_EXT1__ 1
in line 35. probably we need to move the #include <rcutils/error_handling.h>
to above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a strong opinion on it. I let you decide. I have looked into ROS coding guide. It is based on the Google C++ Style Guide. And in the Google C++ Style Guide, the header order is as follows:
dir2/foo2.h (a header corresponding to your cpp file)
A blank line
C system headers (more precisely: headers in angle brackets with the .h extension), e.g., <unistd.h>, <stdlib.h>.
A blank line
C++ standard library headers (without file extension), e.g., , .
A blank line
Other libraries' .h files.
A blank line
Your project's .h files.
It looks to me that in all project headers, the other project headers shall be included in the end.
@razr thanks for the PR, DCO is failing. could you address it with comments? |
Sorry, I've missed the DSO, will update it. Thank you for pointing it out. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I left one thing that I think we should change.
That said, strnlen_s
doesn't seem to be a widely implemented piece of the C11 standard (it is optional, and apparently most compilers don't implement it). Regular strnlen
can also be trivially implemented by calling memchr(s, '\0', n)
instead. See https://stackoverflow.com/questions/66346502/which-is-most-standard-strnlen-or-strnlen-s for some details on all of this.
My suggestion here is we do something slightly different, and introduce rcutils_strnlen
. It would be implemented by calling memchr
instead, and then we could call that everywhere (this would also let us remove some conditional code in include/rcutils/error_handling.h
. Thoughts?
This is a portable version that uses memchr as its underlying implementation. Signed-off-by: Chris Lalancette <[email protected]>
612f568
to
6349f20
Compare
@clalancette it works on VxWorks. LGTM. |
All right, sounds good. Can you please rebase this PR and add a |
use
strnlen_s
instead ofstrnlen
if available. VxWorks has onlystrnlen_s