Tyker added inline comments.

================
Comment at: clang/include/clang/AST/DeclCXX.h:2579
+    assert(
+        !ES.getExpr() ||
+        CXXConstructorDeclBits.HasTrailingExplicitSpecifier &&
----------------
Rakete1111 wrote:
> Your or needs parens or the disambiguation is wrong.
i don't understand. but there is no need for disambiguation, `a && true == a` 
so this will work regardless of operator priority.


================
Comment at: clang/include/clang/Serialization/ASTReader.h:2435
+    uint64_t Kind = readInt();
+    bool hasExpr = Kind & 0x1;
+    Kind = Kind >> 1;
----------------
Rakete1111 wrote:
> same here.
what is the issue


================
Comment at: clang/lib/Sema/SemaInit.cpp:9361
         // Only consider converting constructors.
-        if (GD->isExplicit())
+        if (!GD->isMaybeNotExplicit())
           continue;
----------------
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).


================
Comment at: clang/test/CXX/temp/temp.deduct.guide/p1.cpp:74
 virtual A(int(&)[28]) -> A<int>; // expected-error {{'virtual' can only appear 
on non-static member functions}}
-const A(int(&)[28]) -> A<int>; // expected-error {{deduction guide cannot be 
declared 'const'}}
+const A(int(&)[31]) -> A<int>; // expected-error {{deduction guide cannot be 
declared 'const'}}
 
----------------
Rakete1111 wrote:
> Is there a reason why you changed this?
yes. One of change that was made is making deduction guide redeclaration be an 
error. Without this change, this line was a redeclartion so the test didn't 
serve its purpose.


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

Reply via email to