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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
