[PATCH] D49439: [Sema] Fix a crash while converting constructors to deduction guides

Erik Pilkington via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Jul 25 11:06:28 PDT 2018


erik.pilkington added inline comments.


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:1899-1907
     // Canonicalize the type. This (for instance) replaces references to
     // typedef members of the current instantiations with the definitions of
     // those typedefs, avoiding triggering instantiation of the deduced type
     // during deduction.
     // FIXME: It would be preferable to retain type sugar and source
     // information here (and handle this in substitution instead).
     NewDI = SemaRef.Context.getTrivialTypeSourceInfo(
----------------
rsmith wrote:
> This is really the problem: we shouldn't be doing a full canonicalization step here. I expect that even with your patch applied we'll still see problems for cases like:
> ```
> template<unsigned long> struct X {};
> template<typename T> struct A {
>   A(T t, X<sizeof(t)>);
> };
> A a(0, {});
> template<typename U> struct B {
>   B(U u, X<sizeof(u)>);
> };
> B b(0, {});
> ```
> ... because we'll canonicalize the second parameter of `B`'s deduction guide to have type `X<sizeof(t)>` (where that's the `t` from `A`'s deduction guide).
> 
> So I think we should look at fixing the FIXME here properly. There seem to be at least two viable options:
> 
> 1) don't canonicalize the type; instead, extend template instantiation to be able to cope with one template referring to members of another template directly, without instantiating the class containing those members, or
> 2) add a custom `TreeTransform` to do the canonicalization we want to do here, and to avoid the canonicalization we don't want to do
> 
> Both of these seem pretty tricky to get right, though, which is why we currently use the canonicalization hack :(
Yep, that still crashes :/

I started to implement 2 in the new patch. This implementation just unwraps typedefs into the deduction guide, but that is already enough to pass libcxx's test suite. This doesn't handle everything that we could, such as expression in a decltype [1]. This is fine for now though, because the canonicalization hack doesn't either. In fact, I couldn't find any cases where this patch fails but the canonicalization succeeds. I'm inclined to fix the crash now, address any extra cases in a follow-up if we actually want to support them. Does this seem reasonable to you?

It also seems we're reading pretty far between the lines of the standard here, do you think a DR should be filed?

[1]: I think we could support this is we wanted by stubbing out DeclRefExprs to members for a new (or existing?) opaque reference AST node that acted like a declval<T>() analog. This would allow us to do sfinae using the member's type without relying on it's context.


https://reviews.llvm.org/D49439





More information about the cfe-commits mailing list