erichkeane marked an inline comment as done. erichkeane added a comment. In D80743#2074537 <https://reviews.llvm.org/D80743#2074537>, @rsmith wrote:
> In D80743#2074121 <https://reviews.llvm.org/D80743#2074121>, @erichkeane > wrote: > > > @rsmith I think this implements what you've suggested? > > > Yes, thanks. > > > This seems to 'work' for a small subset of works, but it doesn't properly > > register the typedef to the LocalInstantiationScope, so the normal template > > instantiation (like here > > https://github.com/llvm/llvm-project/blob/master/clang/test/SemaCXX/cxx1z-class-template-argument-deduction.cpp#L156) > > ends up hitting the 'findInstantiationOf' assert here: > > https://github.com/llvm/llvm-project/blob/master/clang/lib/Sema/SemaTemplateInstantiate.cpp#L3564 > > Hm. Yes, that'd be a problem. To fix that, we'll need to transform the > typedef declarations as part of transforming the deduction guide. > Roughly-speaking, we've transformed > > template<typename T> struct A { > using U = X1<T>; > A(X2<U>); > }; > > > into something like > > template<typename T> A(X2<U>) -> A<T> { > using U = X1<T>; > }; > > > ... and the problem is that we need to substitute into the new `using U =` > typedef before we form a use of `U` when producing the substituted type of > the alias declaration. > > There are a couple of ways we could approach this: > > 1. when we start transforming a deduction guide, we can walk its > `TypedefDecl` children, and instantiate those into new typedef declarations > immediately, so that later references to them work > 2. when we reach the reference to the typedef declaration (in > `FindInstantiatedDecl`, when we see a declaration whose decl context is a > deduction guide), instantiate the declaration then > > These are distinguishable by an example such as: > > ``` template<typename T> struct Type { using type = typename T::type; }; > struct B { using type = int; }; template<typename T = B> struct A { using > type = Type<T>::type; A(T, typename T::type, type); // #1 A(...); // #2 }; A > a(1, 2, 3); ``` > > For which option 1 would result in a hard error when substituting T=int > into the injected typedef for #1, but option 2 would accept, because > substitution stops at the 'typename T::type' without ever reaching the > typedef. > > For that reason I think option 2 is the way to go. We already have some > logic for this in `FindInstantiatedDecl`; search there for `NeedInstantiate` > and try extending it from local classes and enums to also cover typedefs > whose decl context is a deduction guide. (You'll need a matching change in > `findInstantiationOf` to return `nullptr` in that case. This code doesn't > seem very well factored...) Patch incoming! option 2 ended up being pretty easy to implement (perhaps embarassingly so on my part after your explaination, I can't help but think I should have put that together), and it results in it passing all the tests. Thanks so much for your patience. ================ Comment at: clang/lib/Sema/SemaTemplate.cpp:1969-1970 + SemaRef.getASTContext(), + SemaRef.getASTContext().getTranslationUnitDecl(), SourceLocation(), + SourceLocation(), TL.getTypedefNameDecl()->getIdentifier(), TSI); + MaterializedTypedefs.push_back(Decl); ---------------- rsmith wrote: > I think it would be a good idea to retain the source location from the > original typedef (and to produce a `TypeAliasDecl` if that's what we started > with); if any diagnostics end up referring to this typedef, we will want them > to point to the one in the class template (and describe it as a "typedef > declaration" or "alias declaration" as appropriate). Ah, right! I had a TODO in my notebook for the source locations, but didn't think about TypeAliasDecl, though I should have. Looks like it shouldbe easy enough, thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80743/new/ https://reviews.llvm.org/D80743 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
