On Thu, Jun 16, 2011 at 11:36 PM, Douglas Gregor <[email protected]> wrote:
> > On Jun 15, 2011, at 10:11 AM, Manuel Klimek wrote: > > On Tue, Jun 14, 2011 at 8:21 PM, Douglas Gregor <[email protected]> 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()[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...) > > > 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. > Now I feel like I'm really missing something here... In the previous code: if (Kind.getKind() == InitializationKind::IK_Copy || > Kind.isExplicitCast()) > return ExprResult(Args.release()[0]); ... reads to me as: if (condition) return statement; <... some code that was never executed ...> which I (at least hope that I did) transformed to: assert(condition); return statement; <... delete code that was never executed ...> which I would think is equivalent, given that the assert always holds true. What am I missing? Cheers, /Manuel 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? > > > VDecl->getType().getNonReferenceType(). > > - Doug > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
