Changed to always require a type in ParenListExpr and tried to fill in the
right types (sanity checking welcome :)
On Fri, Jun 17, 2011 at 9:15 AM, Douglas Gregor <[email protected]> wrote:
>
> On Jun 17, 2011, at 8:11 AM, Douglas Gregor wrote:
>
>
> On Jun 16, 2011, at 11:47 PM, Manuel Klimek wrote:
>
> 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?
>
>
> The code that is currently never executed is still nonetheless correct, and
> a minor tweak the any of the callers' handling of initialization involving
> type-dependent expressions could make this code relevant again. If we take
> your simplification, and one of those tweaks happen, we'll silently
> transform ASTs into something semantically different and cause ourselves
> some serious debugging pain.
>
> So if you want to simplify this code, I'm asking you to add appropriate
> assertions to make sure that we catch the cases where this code will be
> broken. In particular, the code itself is designed to handle NumArgs != 1,
> but your simplification assumes NumArgs == 1 without checking.
>
>
> Apparently, I'm just misreading patches right now. This change is fine;
> sorry for the confusion.
>
> - Doug
>
>
diff --git a/include/clang/AST/Expr.h b/include/clang/AST/Expr.h
index 0a6ad28..0fa0550 100644
--- a/include/clang/AST/Expr.h
+++ b/include/clang/AST/Expr.h
@@ -3752,7 +3752,7 @@ class ParenListExpr : public Expr {
public:
ParenListExpr(ASTContext& C, SourceLocation lparenloc, Expr **exprs,
- unsigned numexprs, SourceLocation rparenloc);
+ unsigned numexprs, SourceLocation rparenloc, QualType T);
/// \brief Build an empty paren list.
explicit ParenListExpr(EmptyShell Empty) : Expr(ParenListExprClass, Empty) { }
diff --git a/lib/AST/Expr.cpp b/lib/AST/Expr.cpp
index cc2bdf5..886c925 100644
--- a/lib/AST/Expr.cpp
+++ b/lib/AST/Expr.cpp
@@ -2990,11 +2990,11 @@ void DesignatedInitExpr::ExpandDesignator(ASTContext &C, unsigned Idx,
ParenListExpr::ParenListExpr(ASTContext& C, SourceLocation lparenloc,
Expr **exprs, unsigned nexprs,
- SourceLocation rparenloc)
- : Expr(ParenListExprClass, QualType(), VK_RValue, OK_Ordinary,
+ SourceLocation rparenloc, QualType T)
+ : Expr(ParenListExprClass, T, VK_RValue, OK_Ordinary,
false, false, false),
NumExprs(nexprs), LParenLoc(lparenloc), RParenLoc(rparenloc) {
-
+ assert(!T.isNull() && "ParenListExpr must have a valid type");
Exprs = new (C) Stmt*[nexprs];
for (unsigned i = 0; i != nexprs; ++i) {
if (exprs[i]->isTypeDependent())
diff --git a/lib/Sema/SemaDeclCXX.cpp b/lib/Sema/SemaDeclCXX.cpp
index 53b5ad2..1ea4a47 100644
--- a/lib/Sema/SemaDeclCXX.cpp
+++ b/lib/Sema/SemaDeclCXX.cpp
@@ -1610,7 +1610,8 @@ 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,
+ Member->getType().getNonReferenceType());
DiscardCleanupsInEvaluationContext();
} else {
@@ -1646,8 +1647,9 @@ Sema::BuildMemberInitializer(ValueDecl *Member, Expr **Args,
// initializer. However, deconstructing the ASTs is a dicey process,
// 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);
+ Init = new (Context) ParenListExpr(
+ Context, LParenLoc, Args, NumArgs, RParenLoc,
+ Member->getType().getNonReferenceType());
else
Init = MemberInit.get();
}
@@ -1703,22 +1705,7 @@ Sema::BuildDelegatingInitializer(TypeSourceInfo *TInfo,
if (DelegationInit.isInvalid())
return true;
- // If we are in a dependent context, template instantiation will
- // perform this type-checking again. Just save the arguments that we
- // received in a ParenListExpr.
- // FIXME: This isn't quite ideal, since our ASTs don't capture all
- // of the information that we have about the base
- // initializer. However, deconstructing the ASTs is a dicey process,
- // and this approach is far more likely to get the corner cases right.
- if (CurContext->isDependentContext()) {
- ExprResult Init
- = Owned(new (Context) ParenListExpr(Context, LParenLoc, Args,
- NumArgs, RParenLoc));
- return new (Context) CXXCtorInitializer(Context, Loc, LParenLoc,
- Constructor, Init.takeAs<Expr>(),
- RParenLoc);
- }
-
+ assert(!CurContext->isDependentContext());
return new (Context) CXXCtorInitializer(Context, Loc, LParenLoc, Constructor,
DelegationInit.takeAs<Expr>(),
RParenLoc);
@@ -1803,7 +1790,8 @@ Sema::BuildBaseInitializer(QualType BaseType, TypeSourceInfo *BaseTInfo,
// any of the arguments are type-dependent expressions.
ExprResult BaseInit
= Owned(new (Context) ParenListExpr(Context, LParenLoc, Args, NumArgs,
- RParenLoc));
+ RParenLoc,
+ BaseType.getNonReferenceType()));
DiscardCleanupsInEvaluationContext();
@@ -1861,7 +1849,8 @@ Sema::BuildBaseInitializer(QualType BaseType, TypeSourceInfo *BaseTInfo,
if (CurContext->isDependentContext()) {
ExprResult Init
= Owned(new (Context) ParenListExpr(Context, LParenLoc, Args, NumArgs,
- RParenLoc));
+ RParenLoc,
+ BaseType.getNonReferenceType()));
return new (Context) CXXCtorInitializer(Context, BaseTInfo,
BaseSpec->isVirtual(),
LParenLoc,
@@ -7613,9 +7602,9 @@ void Sema::AddCXXDirectInitializerToDecl(Decl *RealDecl,
// Store the initialization expressions as a ParenListExpr.
unsigned NumExprs = Exprs.size();
- VDecl->setInit(new (Context) ParenListExpr(Context, LParenLoc,
- (Expr **)Exprs.release(),
- NumExprs, RParenLoc));
+ VDecl->setInit(new (Context) ParenListExpr(
+ Context, LParenLoc, (Expr **)Exprs.release(), NumExprs, RParenLoc,
+ VDecl->getType().getNonReferenceType()));
return;
}
diff --git a/lib/Sema/SemaExpr.cpp b/lib/Sema/SemaExpr.cpp
index a008776..f8962ea 100644
--- a/lib/Sema/SemaExpr.cpp
+++ b/lib/Sema/SemaExpr.cpp
@@ -5847,7 +5847,8 @@ ExprResult Sema::ActOnParenOrParenListExpr(SourceLocation L,
if (nexprs == 1 && TypeOfCast && !TypeIsVectorType(TypeOfCast))
expr = new (Context) ParenExpr(L, R, exprs[0]);
else
- expr = new (Context) ParenListExpr(Context, L, exprs, nexprs, R);
+ expr = new (Context) ParenListExpr(Context, L, exprs, nexprs, R,
+ exprs[nexprs-1]->getType());
return Owned(expr);
}
diff --git a/lib/Sema/SemaInit.cpp b/lib/Sema/SemaInit.cpp
index 5bdadc6..1b71435 100644
--- a/lib/Sema/SemaInit.cpp
+++ b/lib/Sema/SemaInit.cpp
@@ -3915,19 +3915,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.
diff --git a/test/Sema/paren-list-expr-type.cpp b/test/Sema/paren-list-expr-type.cpp
new file mode 100644
index 0000000..ad5b7fb
--- /dev/null
+++ b/test/Sema/paren-list-expr-type.cpp
@@ -0,0 +1,17 @@
+// RUN: %clang -cc1 -ast-dump %s | not grep NULL
+// Makes sure that we don't introduce null types when handling
+// ParenListExpr.
+
+template<typename T> class X { void f() { X x(*this); } };
+
+template<typename T> class Y { Y() : t(1) {} T t; };
+
+template<typename T> class Z { Z() : b(true) {} const bool b; };
+
+template<typename T> class A : public Z<T> { A() : Z<T>() {} };
+
+class C {};
+template<typename T> class D : public C { D(): C() {} };
+
+void f() { (int)(1, 2); }
+
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
