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

[Richard Smith via cfe-commits]

rsmith added a comment.

In http://reviews.llvm.org/D19851#420762, @nicholas wrote:

> I did not expand this to SK_BindReferenceToTemporary, please review this decision. It's also missing missing bit-field and vector element checks that SK_BindReference has.


That's fine. SK_BindReferenceToTemporary can never bind to a dereferenced null pointer.


================
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()))
----------------
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.

================
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()) {
----------------
It seems -- borderline -- worth factoring out these four lines between here and SemaExpr. Maybe `isProvablyEmptyLvalue` or similar, if you feel inclined to do so?

================
Comment at: lib/Sema/SemaInit.cpp:3523
@@ +3522,3 @@
+          isNullPointerConstant(S.Context, Expr::NPC_ValueDependentIsNotNull) &&
+        !UO->getType().isVolatileQualified()) {
+    S.DiagRuntimeBehavior(UO->getOperatorLoc(), UO,
----------------
I don't think we need to, or should, care whether the type is volatile-qualified here. The reference binding has undefined behavior either way. In SemaExpr we had this check because we wanted to warn people that we were going to delete their code, which we didn't do in the volatile case, but in this case we may delete the code even if it's a reference-to-volatile.


http://reviews.llvm.org/D19851