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

Richard Smith - zygoloid via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Apr 30 14:40:05 PDT 2019


rsmith added a comment.

This is great, thank you so much!



================
Comment at: clang/include/clang/AST/DeclBase.h:1539-1541
+    uint64_t NumCtorInitializers : 64 - NumDeclContextBits -
+        NumFunctionDeclBits -
+        /*Other used bits in CXXConstructorDecl*/ 3;
----------------
Tyker wrote:
> rsmith wrote:
> > I would prefer that we keep an explicit number here so that we can ensure that this field has the range we desire.
> couldn't we compute the value and static_assert on it. having to modify this each time we modify DeclContextBits or FunctionDeclBits is sad. and there isn't anything reminding us to do so in some cases.
> what would be a reasonable minimum ?
I think it'd be reasonable to reduce this to 20 if you like. (Going down as far as 16 or so seems acceptable, but I'd be worried about generated code having thousands of members, so I'd be hesitant to go below that.) The existing `static_assert` on line ~1721 will then catch if we add too many bit-field members.


================
Comment at: clang/include/clang/AST/DeclCXX.h:2604
 
-  static CXXConstructorDecl *CreateDeserialized(ASTContext &C, unsigned ID,
-                                                bool InheritsConstructor);
+  static CXXConstructorDecl *CreateDeserialized(ASTContext &C, unsigned ID, uint64_t AllocKind);
   static CXXConstructorDecl *
----------------
Please wrap this to 80 columns.


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:6174
                                                         CXXConversionDecl)) {
+  // FIXEME : it's not clear whether this should match a dependent explicit(....)
   return Node.isExplicit();
----------------
Typo "FIXEME" -> "FIXME"


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:6174
                                                         CXXConversionDecl)) {
-  return Node.isExplicit();
+  return Node.getExplicitSpecifier().isSpecified();
 }
----------------
Tyker wrote:
> 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 ?
Yes, it should. This patch is doing a lot of things already, though, so I'd prefer that we fix that as a separate change.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:3906
       } else
         Diag(Tok, DiagID) << PrevSpec;
     }
----------------
These diagnostics need to be updated to use a different location (rather than using `Tok`) if `isAlreadyConsumed`.


================
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:
> > > > 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?


================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:2076
+    Method = CXXConstructorDecl::Create(
+        SemaRef.Context, Record, StartLoc, NameInfo, T, TInfo, InstantiatedExplicitSpecifier,
+        Constructor->isInlineSpecified(), false, Constructor->isConstexpr());
----------------
Please reflow to 80 columns.


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

https://reviews.llvm.org/D60934





More information about the cfe-commits mailing list