[PATCH] D98798: Produce warning for performing pointer arithmetic on a null pointer.

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:10723-10733
+      bool LHSIsNullPtr = LHS.get()->IgnoreParenCasts()->isNullPointerConstant(
+          Context, Expr::NPC_ValueDependentIsNotNull);
+      bool RHSIsNullPtr = RHS.get()->IgnoreParenCasts()->isNullPointerConstant(
+          Context, Expr::NPC_ValueDependentIsNotNull);
+
+      // Subtracting nullptr or from nullptr should produce
+      // a warning expect nullptr - nullptr is valid in C++ [expr.add]p7
----------------
nickdesaulniers wrote:
> Might it be better to move the C++ check to the top; have all of this within a `if (!getLangOpts().CPlusPlus) {` block? Perhaps I'm misunderstanding what the `||` is doing?
The getLangOpts().CPlusPlus check is specifically so we don't warn on null-null, which the C++ standard allows.


================
Comment at: clang/test/Sema/pointer-addition.c:34
+  f = (char*)(f - (char*)0); // expected-warning {{performing pointer arithmetic on a null pointer has undefined behavior}}
+  f = (char*)((char*)0 - (char*)0); // expected-warning {{performing pointer arithmetic on a null pointer has undefined behavior}} expected-warning {{performing pointer arithmetic on a null pointer has undefined behavior}}
 }
----------------
jamieschmeiser wrote:
> jamieschmeiser wrote:
> > efriedma wrote:
> > > This is what I was afraid would happen.
> > > 
> > > C++ has a specific carveout in the standard that "null-null" isn't undefined. C doesn't, but I'm not sure warning is practically useful there.
> > > 
> > > In any case, printing the same warning twice isn't useful.
> > Yes, I see that [expr.add] p 7 says nullptr-nullptr is defined to be 0.  I will suppress the warning for this.
> I disagree with the comment that the two warnings are not useful.  While it is the same warning, the section of code indicated by each warning is different and both need to be corrected to clear the code of warnings.  A single warning would likely be more confusing in that a user would see the same warning and not notice that a different section of code is indicated after fixing the indicated problem.  I would expect the typical response to be the user assuming that they had made a mistake and removing the initial fix and fixing the second, only to again receive a same warning again with a different section of code.  The different code sections indicated would likely be overlooked, leading to frustration whereas two warnings clearly indicates there are 2 problems.
I see what you mean about the two warnings; printing twice is probably okay.

In terms of whether we actually want the null-null warning in C, I could go either way.  I usually like to avoid differences between C and C++ where possible.  But the standards are actually written differently here.  And it's unlikely that explicitly written null-null shows up in practice, so probably nobody will notice either way.

Needs a testcase for the C++ behavior.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98798/new/

https://reviews.llvm.org/D98798