[Sema] No -Wtautological-pointer-compare warning on variables within parentheses

[danielmarjamaki]

Summary

Submitting a patch to Bugzilla 20951.

Simply replaced the function call IgnoreImpCasts to IgnoreParenImpCasts which seems to more appropriate.
I also had to modify a test cast in test/Sema/conditional-expr.c

/E

[lebedev.ri]

test/Sema/conditional-expr.c

84

Please don't just remove previous tests.
E.g. does the old test no longer warns?

[danielmarjamaki]

I think a test for -Wtautological-pointer-compare should be added that shows that the bug is fixed.

test/Sema/conditional-expr.c

84

no test is removed. The expected-warning is unchanged.

the problem with the test was that this comparison is always true:

(&x) != ((void *)0)

the address of x is never 0!

Fixing https://bugs.llvm.org/show_bug.cgi?id=20951 means Clang will warn:

warning: comparison of address of 'x' not equal to a null pointer is always true [-Wtautological-pointer-compare]

We changed the test so the -Wtautological-pointer-compare is not reported... but the original warning is still reported.

[erikv]

Added new test

[danielmarjamaki]

LGTM! However I would like to see a review from somebody else also.

There are a number of diagnostics that might be affected. The Sema::DiagnoseAlwaysNonNullPointer diagnoses these:

diag::warn_this_null_compare
diag::warn_this_bool_conversion
diag::warn_address_of_reference_null_compare
diag::warn_address_of_reference_bool_conversion
diag::warn_nonnull_expr_compare
diag::warn_cast_nonnull_to_bool
diag::warn_null_pointer_compare <-- I think this is the one bug 20951 is about
diag::warn_impcast_pointer_to_bool

It seems to me that it is an improvement for all these warnings to skip the parentheses. However there is a danger that parentheses should hide some warnings to make it possible for users to hide unwanted warnings. But if that was the design decision then some regression test should complain when we skip the parentheses.

[danielmarjamaki]

Erik and I will not continue working on this. Feel free to take over the patch or write a new patch.

lib/Sema/SemaChecking.cpp

void Sema::DiagnoseAlwaysNonNullPointer(Expr *E,
 
   // Don't warn inside macros.
   if (E->getExprLoc().isMacroID()) {
     const SourceManager &SM = getSourceManager();
     if (IsInAnyMacroBody(SM, E->getExprLoc()) ||
         IsInAnyMacroBody(SM, Range.getBegin()))
       return;
   }
-  E = E->IgnoreImpCasts();
+  E = E->IgnoreParenImpCasts();
 
   const bool IsCompare = NullKind != Expr::NPCK_NotNull;
 
   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;

test/Sema/conditional-expr.c

void foo() {
   int __attribute__((address_space(2))) *adr2;
   int __attribute__((address_space(3))) *adr3;
   test0 ? adr2 : adr3; // expected-warning {{pointer type mismatch}} expected-warning {{expression result unused}}
 
   // Make sure address-space mask ends up in the result type
   (test0 ? (test0 ? adr2 : adr2) : nonconst_int); // expected-warning {{pointer type mismatch}} expected-warning {{expression result unused}}
 }
 
-int Postgresql() {
-  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}}
+int Postgresql(char *x) {
+  //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}}
 }
 
 #define nil ((void*) 0)
 
 extern int f1(void);
 
 int f0(int a) {
   // GCC considers this a warning.