[cfe-commits] [PATCH] Fix ParenListExpr with null type

[Manuel Klimek]

On Tue, Jun 14, 2011 at 8:21 PM, Douglas Gregor <dgregor at apple.com> wrote:

>
> On Jun 14, 2011, at 6:06 PM, Manuel Klimek wrote:
>
> > The attached patch
> > - fixes the introduction of null types for ParenListExpr's that end up
> > in the AST for explicit initializers by making the constructor of
> > ParenListExpr take a type (as suggested by dgregor on irc)
> > - gets rid of some code I assume is dead (tested by running the tests
> > and by running it over all of our internal C++ code without hitting
> > any of those asserts - and by my inability to come up with an example
> > that hits that code path (which admittedly doesn't mean a lot))
> > - tested that the other in-use cases that create ParenListExpr's
> > without a type don't lead to them being in the AST in the end
>
> Index: lib/Sema/SemaDeclCXX.cpp
> diff --git a/lib/Sema/SemaDeclCXX.cpp b/lib/Sema/SemaDeclCXX.cpp
> index
> ce99efbd0bd2020f702da71fc87a80d0b86759a8..a95ef69085526291eb7532aeb9788f68bab235f6
> 100644
> --- a/lib/Sema/SemaDeclCXX.cpp
> +++ b/lib/Sema/SemaDeclCXX.cpp
> @@ -1617,7 +1617,7 @@ Sema::BuildMemberInitializer(ValueDecl *Member, Expr
> **Args,
>     // Can't check initialization for a member of dependent type or when
>     // any of the arguments are type-dependent expressions.
>     Init = new (Context) ParenListExpr(Context, LParenLoc, Args, NumArgs,
> -                                       RParenLoc);
> +                                       RParenLoc, QualType());
>
>     // Erase any temporaries within this evaluation context; we're not
>     // going to track them in the AST, since we'll be rebuilding the
>
> Why not just use Member->getType() as the type of the ParenListExpr, so it
> never has a NULL type?
>

Because I wasn't able to get it to make a difference, in which case I
default to not changing it. If you think the invariant should be that the
ParenListExpr always has a type, I'm happy to change this. I'll have a hard
time writing a test for it, though ... ;)


> @@ -1659,7 +1659,7 @@ Sema::BuildMemberInitializer(ValueDecl *Member, Expr
> **Args,
>     // and this approach is far more likely to get the corner cases right.
>     if (CurContext->isDependentContext())
>       Init = new (Context) ParenListExpr(Context, LParenLoc, Args, NumArgs,
> -                                               RParenLoc);
> +                                         RParenLoc, QualType());
>
> Same question here, and in a number of places below. It seems like, if
> we're going to fix this, we should always make sure that ParenListExpr has a
> type.
>
> Index: lib/Sema/SemaInit.cpp
> diff --git a/lib/Sema/SemaInit.cpp b/lib/Sema/SemaInit.cpp
> index
> a33f5d0b2f3ef530c689a2ddc28ebaef3074635e..f0d39893a449e62902f21160f10bd6a400353789
> 100644
> --- a/lib/Sema/SemaInit.cpp
> +++ b/lib/Sema/SemaInit.cpp
> @@ -3684,19 +3684,9 @@ InitializationSequence::Perform(Sema &S,
>
>       }
>     }
> -
> -    if (Kind.getKind() == InitializationKind::IK_Copy ||
> Kind.isExplicitCast())
> -      return ExprResult(Args.release()[0]);
> -
> -    if (Args.size() == 0)
> -      return S.Owned((Expr *)0);
> -
> -    unsigned NumArgs = Args.size();
> -    return S.Owned(new (S.Context) ParenListExpr(S.Context,
> -                                                 SourceLocation(),
> -                                                 (Expr **)Args.release(),
> -                                                 NumArgs,
> -                                                 SourceLocation()));
> +    assert(Kind.getKind() == InitializationKind::IK_Copy ||
> +           Kind.isExplicitCast());
> +    return ExprResult(Args.release()[0]);
>   }
>
>   // No steps means no initialization.
>
> Hrm, interesting. This is only dead because its potential callers seem to
> avoid building InitializationSequences in dependent cases. Please update the
> assert to always check that Args.size() ==1, and if *that* doesn't trigger
> any failures, this change is okay. We can always resurrect the code if the
> callers change.
>

I'm happy to do this, but I don't understand the underlying assumption yet -
even if Args.size() != 1, if the assert that I added is true, the code will
behave exactly the same way as before (before we would early return if it is
true). So I'm not sure what the extra assert would change (if it breaks, and
that makes the code wrong, I'd say the code was wrong before...)

Apart from that I realized that I hadn't run it over all our code in debug
mode - I now started this, and promptly ran into an assert - it's not in the
dead code, though, but in the code that actually adds the type

assert.h assertion failed at
third_party/llvm/trunk/tools/clang/include/clang/AST/Expr.h:83 in void
clang::Expr::setType(clang::QualType): (t.isNull() || !t->isReferenceType())
&& "Expressions can't have reference type"

So apparently sometimes the VDecl->getType() is a reference type...
What's the correct solution here?

Thanks!
/Manuel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110615/2b43a704/attachment.html>