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. - Doug
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
