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
