[PATCH] D60934: [clang] adding explicit(bool) from c++2a

[Tyker via Phabricator via cfe-commits]

Tyker added inline comments.


================
Comment at: clang/include/clang/AST/DeclCXX.h:2033
+
+  void setExplicitSpecifier(ExplicitSpecInfo ESI);
+
----------------
Tyker wrote:
> Tyker wrote:
> > rsmith wrote:
> > > Generally we don't want to have setters in the AST; the AST is intended to be immutable after creation. Is this necessary?
> > this is used in 2 cases:
> > - for deserialization.
> > - for in `Sema::ActOnFunctionDeclarator` to make every declaration of the same function have the same explicit specifier as the canonical one. there is probably a better way to do this but i didn't find how.
> > 
> > the second use case will need to be changed because the constructor's explicit specifier will be tail-allocated so the explicit specifier from the canonical declaration will need to be recovered before construction to allocate storage. 
> > 
> > how can we find the canonical declaration of a function before it is constructed ?
> i found a solution around that issue.
> now setExplicitSpecifier is only used for deserialization.
as it is only used by deserialization and ASTDeclReader is friend to all class with a setExplicitSpecifier.
I made setExplicitSpecifier private.


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:6174
                                                         CXXConversionDecl)) {
-  return Node.isExplicit();
+  return Node.getExplicitSpecifier().isSpecified();
 }
----------------
rsmith wrote:
> This will match `explicit(false)` constructors. I think `getExplicitSpecifier().isExplicit()` would be better, but please also add a FIXME here: it's not clear whether this should match a dependent `explicit(....)`.
shouldn't this be able to match with deduction guides too ?


================
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:
> > > 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`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60934/new/

https://reviews.llvm.org/D60934