[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In https://reviews.llvm.org/D37042#855713, @efriedma wrote:

> I didn't think of this earlier, but strictly speaking, I think "(char*)nullptr+0" isn't undefined in C++?


Yes, that's correct. (C++'s model is basically equivalent to having an object of size zero at the null address, so things like `(char*)nullptr - (char*)nullptr` and `(char*)nullptr + 0` are valid.)

> But probably worth emitting the warning anyway; anyone writing out arithmetic on null is probably doing something suspicious, even if it isn't technically undefined.

I agree. We should probably suppress the warning if the non-pointer operand is known to be zero in C++, and weaken the wording slightly "[..] has undefined behavior if offset is nonzero" otherwise, though.



================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6032
+def ext_gnu_null_ptr_arith : Extension<
+  "inttoptr casting using arithmetic on a null pointer is a GNU extension">,
+  InGroup<PointerArith>;
----------------
efriedma wrote:
> The keyword "inttoptr" is part of LLVM, not something we expect users to understand.
How about something like "arithmetic on null pointer treated as cast from integer to pointer as a GNU extension"?


================
Comment at: lib/Sema/SemaExpr.cpp:8799
 
+  // Adding to a null pointer is never an error, but should warn.
+  if (PExp->IgnoreParenCasts()->isNullPointerConstant(Context, 
----------------
Maybe `// Adding to a null pointer results in undefined behavior.` to explain why we warn (a reader of the code can already see that we do warn in this case).


================
Comment at: lib/Sema/SemaExpr.cpp:8805
+    const PointerType *pointerType = PExp->getType()->getAs<PointerType>();
+    if (!IExp->isIntegerConstantExpr(Context) &&
+        pointerType->getPointeeType()->isCharType() &&
----------------
It seems strange to me to disable this when the RHS is an ICE. If we're going to support this as an extension, we should make the rules for it as simple as we reasonably can; this ICE check seems like an unnecessary complication.


https://reviews.llvm.org/D37042