Add -Wpointer-compare to check instances of comparing a Null pointer to '\0'.
Details
Diff Detail
Event Timeline
+1, useful check.
what about:
char *a...
(char) 0 == a
?
Is this off by default in GCC? I would like to see it -Wall/-Wextra..
clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
---|---|---|
3291 | I think this would read a bit better as comparing a pointer to a null character constant; did you mean to compare to %select{nullptr|NULL|0}? I'm a bit unsure about NULL vs 0 in the diagnostic, but I kind of feel like we should prefer nullptr if it's enabled, NULL if it's available, and 0 only as a last-ditch fallback. | |
3292 | Why is this disabled by default? | |
clang/include/clang/Sema/Sema.h | ||
9994 | Should be CheckNullCharToNullptr() to fit with the surrounding naming conventions. | |
clang/lib/Sema/SemaExpr.cpp | ||
10436 | I'd drop this comment as it doesn't add much value. | |
10441 | Rather than do isa<> followed by a dyn_cast<>, this should be: if (const auto *RCL = dyn_cast<CharacterLiteral>(E.get())) { } | |
10443 | I think it would be more clear to compare against 0 as an integer literal here. | |
clang/test/Sema/warn-nullchar-nullptr.c | ||
10 | I'd appreciate tests for the other character literals as well. L'\0', u8'\0', etc. |
Thanks for the suggestion, I added the test.
Is this off by default in GCC? I would like to see it -Wall/-Wextra..
This is not off by default in GCC, changed.
clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
---|---|---|
3292 | I changed this to not off by default. |
clang/include/clang/Sema/Sema.h | ||
---|---|---|
9994 | I'd drop the comment here, it doesn't add much value (and it's over the 80 col limit). | |
clang/lib/Sema/SemaExpr.cpp | ||
10447 | This logic is incorrect (and it won't pick from three different options provided by the diagnostic). | |
10448–10449 | Formatting is off here -- you should run the patch through clang-format, if you don't already do that. | |
10451 | This should probably be looking at the canonical type so that we strip off all the sugar. You should add a test like: typedef char my_char; int func(int *a) { return a == (my_char)0; } | |
10452 | I see that GCC only warns on char types, but it seems equally plausible (through macros or typedefs) to get a wchar_t or other kind of character type. Should we warn on those as well? | |
clang/test/Sema/warn-nullchar-nullptr.c | ||
5 | We definitely should not be suggesting nullptr in C code. :-) |
clang/lib/Sema/SemaExpr.cpp | ||
---|---|---|
10452 | I don't have any opinion about this. Which one do you think is better? |
clang/test/Sema/warn-nullchar-nullptr.c | ||
---|---|---|
6 | Please add test for: a != ‘\0’ |
clang/lib/Sema/SemaExpr.cpp | ||
---|---|---|
10452 | It works for typedefs and MACRO now. Is this what you expected? |
clang/lib/Sema/SemaExpr.cpp | ||
---|---|---|
10460 | You can ditch SelectValue and use %0 in diag msg. Then ‘<< NullValue’. |
Can you add tests for C++? E.g C++03 and C++11?
Other than that, I have no comments - looks fine.
C++ doesn't support comparison between pointer and character, so I guess it's not necessary?
Here is an example of comparing pointer and character in C++:
int foo(int *a) {
return a == '\0';
}
#1 with x86-64 clang (trunk)
<source>:2:14: error: comparison between pointer and integer ('int *' and 'char')
return a == '\0'; ~ ^ ~~~~
1 error generated.
Compiler returned: 1
So
if (C mode only) {
CheckNullCharToNullptr(LHS, RHS);
CheckNullCharToNullptr(RHS, LHS);
}
?
Drop if (cpp11) condition in the function?
Rename it please. ‘CheckPtrComparisonWithNullChar’ ?
clang/lib/Sema/SemaExpr.cpp | ||
---|---|---|
10499 | !getLangOpts().CPlusPlus ? |
clang/lib/Sema/SemaExpr.cpp | ||
---|---|---|
10439–10443 | I would rather see this done using a %select in the diagnostic, since both of the strings are statically known at compile time. | |
10452 | Not quite -- I wasn't very clear. I was thinking more about a scenario like this: #define CHECK_VAL(x, y) (x != y) #ifdef NARROW #define TYPE char #else #define TYPE wchar_t #endif void func(int *ip) { if (CHECK_VAL(ip, (TYPE)0)) ; } When NARROW is defined, your check will trigger, but when it's not defined, there is no diagnostic. I can't think of a situation where we'd want to warn on a cast involving char but not wchar_t (for instance), so I think you should check for isAnyCharacterType() rather than just CharTy below. | |
10453 | Formatting is still off here (the else binds to the } above). | |
10459 | When switching to using %select above, this can become NullValue ? "NULL" : "(void *)0" | |
10499 | Agreed -- that's the canonical way to check for any C mode. |
@aaron.ballman, Should we also consider address spaces? Or exotic NULL as non-zero bit pattern?
I don't think address spaces would have any impact here, and unless LLVM supports a platform with a non-zero NULL bit pattern, I don't think it would be all that beneficial to consider it (we could always do that in a follow-up patch though).
clang/lib/Sema/SemaExpr.cpp | ||
---|---|---|
10452 | I changed the check to isAnyCharacterType(), but I don't think there's any way to test it in C cause wchar_t will be treated as an interger in C, so I didn't add tests for this. |
a > (char) 0 deserves other diagnostic message.. gcc emits: ordered comparison of pointer with integer zero.
This warning should handle ==, != only, I think.
Pleae add test for a > ‘\0’ to be sure we dont suggest a > NULL.
Then a follow up patch could diagnose ordered comparison with integer zero.
clang/lib/Sema/SemaExpr.cpp | ||
---|---|---|
10439–10443 | int NullValue = PP.isMacroDefined("NULL") ? 0 : 1; | |
10452 | Ugh, so that explains why this part of the check only makes sense for char types. I forgot that wchar_t has an underlying integer type in C. I guess you can go back to just checking CharTy because the same holds true for char16_t and char32_t in C. Sorry about the churn! |
I think this would read a bit better as comparing a pointer to a null character constant; did you mean to compare to %select{nullptr|NULL|0}?
I'm a bit unsure about NULL vs 0 in the diagnostic, but I kind of feel like we should prefer nullptr if it's enabled, NULL if it's available, and 0 only as a last-ditch fallback.