-
Notifications
You must be signed in to change notification settings - Fork 254
Add STRNEQ(), and use it instead of its pattern #1336
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
Conversation
1612a74 to
fbdade0
Compare
fbdade0 to
c7bc9f1
Compare
fc42cc1 to
49a51b7
Compare
39c64af to
cb9791c
Compare
lib/utmp.c
Outdated
|
|
||
| #if defined(HAVE_STRUCT_UTMPX_UT_HOST) | ||
| if ((ut != NULL) && (ut->ut_host[0] != '\0')) { | ||
| if ((ut != NULL) && strncmp(ut->ut_host, "", countof(ut->ut_host)) != 0) { |
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 disagree that this is simpler to read. But also, what's the advantage of
using strncmp here, instead of just strcmp(x, "");
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.
If you really want to not see the byte comparisons (... I like them ...) you
could add an is_empty_string() fn? That would be easier to read.
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 disagree that this is simpler to read.
I agree that is not simpler to read.
strncmp(3) is ugly because of the countof() call, and because of the !=0 comparison. However, this is later changed to use the STRNEQ() macro that has 2 arguments. So, the diff after the entire PR would be:
-if ((ut != NULL) && (ut->ut_host[0] != '\0')) {
+if ((ut != NULL) && !STRNEQ(ut->ut_host, "")) {But also, what's the advantage of using strncmp here, instead of just strcmp(x, "");
strcmp(3) requires that its arguments are strings, but ut->ut_host is not a string; it might not be null terminated.
Thus, using strcmp(3) might result in a buffer overrun. In this specific case, since we won't compare more than one byte, it's "fine", however, it's more hygienic to not do that. Programmers could be confused and think that it could be a string by seeing it being passed to strcmp(3).
Also, I expect that in the future it would even trigger a diagnostic, once glibc starts annotating their functions with the new GCC attribute [[gnu::null_terminated_string_arg()]].
That's why I'm adding strneq(), to compare a nonstring instead of a string.
Another problem with str[n]cmp(3), which is solved with str[n]eq(), is the confusing return value.
f you really want to not see the byte comparisons (... I like them ...)
The thing about byte comparisons is that they're hard to grep(1) for. Also, it's hard to know what kind of thing you're comparing, and why. If you see strneq(), you know your comparing a nonstring. If you see streq(), you see you're comparing a string.
you
could add an is_empty_string() fn? That would be easier to read.
We'd need an is_empty_nonstring() for this case. I think comparing to "" for equality is robust enough that I don't see it very compelling to add something like str[n]empty(). Maybe we could discuss that after all of this.
|
Could we replace the !STRNEQ(ut->ut_host, "")) etc with something like !strempty(ut->ut_host)? |
strempty() would replace streq(). For STRNEQ(), we'd need STRNEMPTY(). I'm open to that. However, are you sure you'd find it more readable? It's one more layer to look up, and streq() / STRNEQ() has more chances of being a widely-known API. About the uppercase macros for arrays and your dislike of shouting names, I've been thinking recently, and found some consistent way to name them, that I like better than uppercase: an |
This is simpler to read, IMO. Signed-off-by: Alejandro Colomar <alx@kernel.org>
This allows using __has_c_attribute() in compilers that don't have it. Signed-off-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
cb9791c to
6ef63f6
Compare
These APIs are like streq() and strprefix(), but for nonstrings (thus, the usual strn*() name).
The first parameter is a nonstring, and the second is a string.
Revisions:
v2
v2b
v3c
v4
[[gnu::nonstring]].v4b
v4c