[PATCH] D19851: Warn on binding reference to null in copy initialization

[Nick Lewycky via cfe-commits]

nicholas marked 2 inline comments as done.

================
Comment at: lib/Sema/SemaInit.cpp:3514-3518
@@ +3513,7 @@
+static void CheckForNullPointerDereference(Sema &S, const Expr *E) {
+  // Check to see if we are dereferencing a null pointer.  If so,
+  // and if not volatile-qualified, this is undefined behavior that the
+  // optimizer will delete, so warn about it.  People sometimes try to use this
+  // to get a deterministic trap and are surprised by clang's behavior.  This
+  // only handles the pattern "*null", which is a very syntactic check.
+  if (const UnaryOperator *UO = dyn_cast<UnaryOperator>(E->IgnoreParenCasts()))
----------------
rsmith wrote:
> This comment doesn't make sense for this case. Binding a reference to a dereferenced null pointer isn't something that people would expect to trap. But the idea is the same: we might delete people's code and they're probably not expecting that.
We don't delete it, generally we "create a null reference" which is exactly what some programmers expect, when they think that references are just syntactically different pointers.

================
Comment at: lib/Sema/SemaInit.cpp:3519-3522
@@ +3518,6 @@
+  // only handles the pattern "*null", which is a very syntactic check.
+  if (const UnaryOperator *UO = dyn_cast<UnaryOperator>(E->IgnoreParenCasts()))
+    if (UO->getOpcode() == UO_Deref &&
+        UO->getSubExpr()->IgnoreParenCasts()->
+          isNullPointerConstant(S.Context, Expr::NPC_ValueDependentIsNotNull) &&
+        !UO->getType().isVolatileQualified()) {
----------------
rsmith wrote:
> It seems -- borderline -- worth factoring out these four lines between here and SemaExpr. Maybe `isProvablyEmptyLvalue` or similar, if you feel inclined to do so?
I'll pass. Initially I thought the two CheckForNullPointerDereference functions might be the same but for a diag::ID argument, but they're growing more and more different.

As for `isProvablyEmptyLvalue` we would still need to get the UO for the diagnostic anyways.


http://reviews.llvm.org/D19851