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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
