[cfe-commits] Handle ExprWithCleanups when creating goto scopes

[Douglas Gregor]

On Sep 11, 2012, at 10:16 PM, Rafael Espíndola <rafael.espindola at gmail.com> wrote:

>> The logic of this routine is fairly broken. It's trying to find an object construction (which is doesn't do correctly, even after your patch) and using that to blacklist all of the ill-formed patterns, rather than detecting based on the type and then white-listing the few exceptions.
>> 
>> It should really check for the valid cases directly (scalar/vector types are okay, class types are okay when we're using the trivial default ctor and the dtor is trivial) and fail with a generic error if our type is not one of those valid cases. That way, the restricted form of the checking for CXXConstructExpr that's currently being used will suffice for detecting the "constructed with trivial constructor" case.
> 
> I think we still need to find object construction to decide if we need
> an OutDiag or not.

Ah, yes, that's correct. There's also a terrible case that your patch doesn't get right, which is something like:

	struct X { operator &int() const; }
	const int &i = X();

The problem here, which I hadn't been thinking about before, is lifetime extension for temporaries. We actually have to dig into the initializer expression to find the CXXConstructExpr, even when the variable type is a reference to a scalar type.

> The attached patch improves the existing logic by
> getting the CXXRecordDecl from the type, computing OutDiag early and
> having a white list for InDiag as you suggested.


+      // If the variable is not of record type (or array thereof) the
+      // initializer cannot be a trivial ctor and there is no destructor
+      // to worry about.
+      const Type *T = Init->getType().getTypePtr();
+      if (T->isArrayType())
+        T = T->getBaseElementTypeUnsafe();
+      const CXXRecordDecl *Record = T->getAsCXXRecordDecl();
+      if (!Record)
+        return ScopePair(diag::note_protected_by_variable_init, 0);
+

This early exit seems not to work, per my example above.

+      // If we need to call a non trivial destructor for this variable,
+      // record an out diagnostic.
+      unsigned OutDiag = 0;
+      if (!Record->hasTrivialDestructor() && !isa<DeclRefExpr>(Init))
+        OutDiag = diag::note_exits_dtor;

I don't see what the DeclRefExpr check is meant to do.


FWIW, here's my test from above in a form that you can add to your patch:

namespace test13 {
  struct C {
    C(int x);
    ~C();

    operator int&() const;
  };
  void f(void **ip) {
    static void *ips[] = { &&l0 };
  l0: // expected-note {{possible target of indirect goto}}
    const int &c1 = C(1); // expected-note {{jump exits scope of variable with non-trivial destructor}}
    goto *ip; // expected-error {{indirect goto might cross protected scopes}}
  }
}

  - Doug