[cfe-commits] [PATCH] Fix ParenListExpr with null type
dgregor at apple.com
Thu Jun 16 23:36:51 PDT 2011
On Jun 15, 2011, at 10:11 AM, Manuel Klimek wrote:
> 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 ... ;)
I just figured that if you're going to fix the problem in one place, it would be better to just make it an AST invariant.
> 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());
> - 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());
> // 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...)
The previous code would return *all* of the arguments in Args (NumArgs of them) as a ParenListExpr, indicating direct initialization.
The new code only returns the first of the arguments, so if we're going to make that change, we need to assert that there is exactly one argument.
> 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?
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the cfe-commits