Update with warning message for comparison to NULL pointer

[Krishna-13-cyber]

Summary

This patch solves the issue pointed out in https://github.com/llvm/llvm-project/issues/42992.
While checking the testcases it fails in one of the cases, I am unsure of the fix of that while rest all seems fine.

[Krishna-13-cyber]

• Apologies for uploading some other/incorrect diff
• Updated Diff

[tbaeder]

Both test failures look like you just need to add the additional new diagnostic.

[Krishna-13-cyber]

• Update with diagnostic addition

[Krishna-13-cyber]

• Update with addition of testcase
• There is an issue for which the build fails, actually when I replace the diagnostic in /clang/test/Sema/conditional-expr.c Line 89, it tells C99 forbids conditional expressions with only one void side and if I remove it shows seen but not expected and vice versa.It regresses either ways.

In the same manner for comparison of address of 'g' equal to a null pointer is always false.

[shafik]

clang/test/Sema/warn-tautological-compare.c

102 ↗

(On Diff #516397)

Please add back the missing newline

clang/test/SemaCXX/constant-expression-cxx2a.cpp

252

I don't believe we should be emitting a diagnostic for this case. The static_assert passes. CC @aaron.ballman

[Krishna-13-cyber]

• Updated with reviewed changes

[aaron.ballman]

clang/test/Sema/conditional-expr.c

89

The C99 warning should not be dropped -- that was a valid pedantic diagnostic: https://godbolt.org/z/Tz3zGY8d4

clang/test/SemaCXX/constant-expression-cxx2a.cpp

252

Agreed, this new diagnostic should not be triggered

[Krishna-13-cyber]

• Updated with re-adding the required diagnostic message

[aaron.ballman]

Precommit CI is still failing on test/SemaCXX/constant-expression-cxx2a.cpp with a false positive warning. You'll need to investigate why that's happening, but I suspect that the warning logic isn't aware that dynamic_cast can return null.

clang/lib/Sema/SemaChecking.cpp

14726

clang/test/Sema/warn-tautological-compare.c

97–100 ↗

(On Diff #517151)

[aaron.ballman]

clang/lib/Sema/SemaChecking.cpp

14726

The issue with the failing precommit CI test is because you're ignoring parens and casts with your changes, and to resolve the problem from the issue you should be ignoring parens alone.

Try replacing IgnoreParenCasts() with IgnoreParens() and things should work better.

[Krishna-13-cyber]

• Update with the given suggestion
• Add release notes

Thanks a lot @aaron.ballman for the instant assistance.

[aaron.ballman]

LGTM! There's still a formatting issue in warn-tautological-compare.c but I can fix that up when landing, assuming you still need me to land this on your behalf. (If you can land it on your own, then feel free to fix the formatting issue when you land it.)

[Krishna-13-cyber]

In D149000#4420607, @aaron.ballman wrote:

LGTM! There's still a formatting issue in warn-tautological-compare.c but I can fix that up when landing, assuming you still need me to land this on your behalf. (If you can land it on your own, then feel free to fix the formatting issue when you land it.)

Yes, I don't have the commit access to the repository, It would be great if you could land it on my behalf. Apologies for missing out on the formatting issues.
Thanks a lot!

clang/lib/Sema/SemaChecking.cpp

if (isa<CXXThisExpr>(E)) {
     unsigned DiagID = IsCompare ? diag::warn_this_null_compare
                                 : diag::warn_this_bool_conversion;
     Diag(E->getExprLoc(), DiagID) << E->getSourceRange() << Range << IsEqual;
     return;
   }
 
   bool IsAddressOf = false;
 
-  if (UnaryOperator *UO = dyn_cast<UnaryOperator>(E)) {
+  if (UnaryOperator *UO = dyn_cast<UnaryOperator>(E->IgnoreParenCasts())) {
     if (UO->getOpcode() != UO_AddrOf)
       return;
     IsAddressOf = true;
     E = UO->getSubExpr();
   }
 
   if (IsAddressOf) {
     unsigned DiagID = IsCompare

clang/test/Sema/conditional-expr.c

void foo(void) {
   test0 ? adr2 : adr3; // expected-error{{conditional operator with the second and third operands of type  ('__attribute__((address_space(2))) int *' and '__attribute__((address_space(3))) int *') which are pointers to non-overlapping address spaces}}
 
   // Make sure address-space mask ends up in the result type
   (test0 ? (test0 ? adr2 : adr2) : nonconst_int); // expected-error{{conditional operator with the second and third operands of type  ('__attribute__((address_space(2))) int *' and 'int *') which are pointers to non-overlapping address spaces}}
 }
 
 int Postgresql(void) {
   char x;
-  return ((((&x) != ((void *) 0)) ? (*(&x) = ((char) 1)) : (void) ((void *) 0)), (unsigned long) ((void *) 0)); // expected-warning {{C99 forbids conditional expressions with only one void side}}
+  return ((((&x) != ((void *) 0)) ? (*(&x) = ((char) 1)) : (void) ((void *) 0)), (unsigned long) ((void *) 0)); // expected-warning {{comparison of address of 'x' not equal to a null pointer is always true}}
 }
 
 #define nil ((void*) 0)
 
 extern int f1(void);
 
 int f0(int a) {
   // GCC considers this a warning.

clang/test/SemaCXX/constant-expression-cxx2a.cpp

namespace DynamicCast {
   struct Padding { virtual void padding(); };
   struct G : Padding, F {};
 
   constexpr G g;
 
   // During construction of C, A is unambiguous subobject of dynamic type C.
   static_assert(g.c == (C*)&g);
   // ... but in the complete object, the same is not true, so the runtime fails.
-  static_assert(dynamic_cast<const A*>(static_cast<const C2*>(&g)) == nullptr);
+  static_assert(dynamic_cast<const A*>(static_cast<const C2*>(&g)) == nullptr); // expected-warning {{comparison of address of 'g' equal to a null pointer is always false}}
 
   // dynamic_cast<void*> produces a pointer to the object of the dynamic type.
   static_assert(g.f == (void*)(F*)&g);
   static_assert(dynamic_cast<const void*>(static_cast<const D*>(&g)) == &g);
 
   // expected-note@+1 {{reference dynamic_cast failed: 'A' is an ambiguous base class of dynamic type 'DynamicCast::G' of operand}}
   constexpr int d_a = (dynamic_cast<const A&>(static_cast<const D&>(g)), 0); // expected-error {{}}