rsmith added inline comments.
================ Comment at: clang/lib/Sema/SemaInit.cpp:9361 // Only consider converting constructors. - if (GD->isExplicit()) + if (!GD->isMaybeNotExplicit()) continue; ---------------- Tyker wrote: > rsmith wrote: > > Tyker wrote: > > > rsmith wrote: > > > > Tyker wrote: > > > > > rsmith wrote: > > > > > > We need to substitute into the deduction guide first to detect > > > > > > whether it forms a "converting constructor", and that will need to > > > > > > be done inside `AddTemplateOverloadCandidate`. > > > > > similarly as the previous if. this check removes deduction guide that > > > > > are already resolve to be explicit when we are in a context that > > > > > doesn't allow explicit. > > > > > every time the explicitness was checked before my change i replaced > > > > > it by a check that removes already resolved explicit specifiers. > > > > Unlike in the previous case, we do //not// yet pass an `AllowExplicit` > > > > flag into `AddTemplateOverloadCandidate` here, so this will incorrectly > > > > allow dependent //explicit-specifier//s that evaluate to `true` in > > > > copy-initialization contexts. > > > the default value for `AllowExplicit` is false. so the conversion will be > > > removed in `AddTemplateOverloadCandidate`. > > That doesn't sound right: that'd mean we never use explicit deduction > > guides (we never pass `AllowExplicit = true` to > > `AddTemplateOverloadCandidate`). Do we have any test coverage that > > demonstrates that conditionally-explicit deduction guides are handled > > properly? > my mistake. the default value for AllowExplicit is false. but > AddTemplateOverloadCandidate will only remove conversions and constructors. > dependent explicit specifier that are resolved to true on deduction guides > are removed at line 9480. they are not removed from the overload set. CTAD > just fails if a explicit deduction guide is selected in a CopyInitList. i > agree this is weird but the behavior is the same as before the patch. > the result on clang is: > ``` > template<typename T> > struct A { > explicit A(T); > }; > A a = 0; // error with explicit constructor meaning CTAD succeed. > A a = { 0 }; // error with explicit deduction guide > ``` > all compiler don't agree on this https://godbolt.org/z/ElHlkE. icc and clang > have this behavior, gcc and msvc fail at CTAD time on both initialization. as > of what the standard say from what i read, it isn't clear, the standard is > clear when calling an explicit constructor should fail. but it doesn't appear > to say for deduction guides. > as this was the previous behavior i did not change it with explicit(bool). > the standard is clear when calling an explicit constructor should fail. but > it doesn't appear to say for deduction guides The standard says that you take the set of deduction guides and notionally form a set of constructors from them (see [over.match.class.deduct]). So the constructor rules apply to deduction guides too. > as this was the previous behavior i did not change it with explicit(bool). I don't think that's correct. We filter out explicit deduction guides for non-list copy-initialization on line ~9239 (prior to this change). Today we get this result: ``` template<typename T> struct X { X(int); }; explicit X(int) -> X<int>; // #1 X a = 0; // error: no viable deduction guide, #1 is not a candidate X b = {0}; // error: selected explicit deduction guide #1 X c{0}; // ok X d(0); // ok ``` ... which is correct. If we change the deduction guide to have a dependent `explicit(bool)` specifier: ``` template<bool E = true> explicit(E) X(int) -> X<int>; ``` ... we should get the same result, but I think we won't get that result with this patch: I think we'll incorrectly select an explicit deduction guide for the declaration of `a`, because we never filter out explicit deduction guides. `DeduceTemplateSpecializationFromInitializer` should compute an `AllowExplicit` flag (`= !Kind.isCopyInit() || ListInit`), pass it into `AddTemplateOverloadCandidate`, and that should filter out explicit deduction guide specializations if it's `true`. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60934/new/ https://reviews.llvm.org/D60934 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits