[PATCH] D60934: [clang] adding explicit(bool) from c++2a
Richard Smith - zygoloid via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Apr 29 17:22:51 PDT 2019
rsmith marked 2 inline comments as done.
rsmith added a comment.
Thanks, this is looking really good. (Lots of comments but they're mostly mechanical at this point.)
================
Comment at: clang/include/clang/AST/DeclBase.h:1539-1541
+ uint64_t NumCtorInitializers : 64 - NumDeclContextBits -
+ NumFunctionDeclBits -
+ /*Other used bits in CXXConstructorDecl*/ 3;
----------------
I would prefer that we keep an explicit number here so that we can ensure that this field has the range we desire.
================
Comment at: clang/include/clang/AST/DeclBase.h:1544-1548
+ /// Wether this constructor has a trail-allocated explicit specifier.
+ uint64_t hasTrailExplicit : 1;
+ /// If this constructor does't have a trail-allocated explicit specifier.
+ /// Wether this constructor is explicit specified.
+ uint64_t isExplicitWhenNotTail : 1;
----------------
Typo "Wether" (x2).
================
Comment at: clang/include/clang/AST/DeclBase.h:1545-1548
+ uint64_t hasTrailExplicit : 1;
+ /// If this constructor does't have a trail-allocated explicit specifier.
+ /// Wether this constructor is explicit specified.
+ uint64_t isExplicitWhenNotTail : 1;
----------------
Maybe `HasSimpleExplicit`, `HasTrailingExplicitSpecifier` would be better names? (Please capitalize these to match the surrounding style.)
================
Comment at: clang/include/clang/AST/DeclCXX.h:1995
+ : ExplicitSpec(Expression, Flag) {}
+ ExplicitSpecFlag getFlag() const { return ExplicitSpec.getInt(); }
+ const Expr *getExpr() const { return ExplicitSpec.getPointer(); }
----------------
Usually we'd call this `Kind`, not `Flag`.
================
Comment at: clang/include/clang/AST/DeclCXX.h:2013
+ /// Return true if the ExplicitSpecifier isn't valid.
+ /// this state occurs after a substitution failures.
+ bool isInvalid() const {
----------------
"this" -> "This".
================
Comment at: clang/include/clang/AST/DeclCXX.h:2018-2020
+ void setExprAndFlag(Expr *E, ExplicitSpecFlag Flag) {
+ ExplicitSpec.setPointerAndInt(E, Flag);
+ }
----------------
Do we need this? `= ExplicitSpecifier(E, Kind)` seems just as good and arguably clearer.
================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:6174
CXXConversionDecl)) {
- return Node.isExplicit();
+ return Node.getExplicitSpecifier().isSpecified();
}
----------------
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(....)`.
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2119
+def err_deduction_guide_redeclared : Error<
+ "redeclaration of a deduction guide">;
def err_deduction_guide_specialized : Error<"deduction guide cannot be "
----------------
Drop the "a" here.
================
Comment at: clang/include/clang/Basic/Specifiers.h:26-28
+ RESOLVED_FALSE,
+ RESOLVED_TRUE,
+ UNRESOLVED,
----------------
My apologies, I led you the wrong way: the style to use for these is `ResolvedFalse`, `ResolvedTrue`, `Unresolved`. (I meant to capitalize only the first letter, not the whole thing, in my prior comment.)
================
Comment at: clang/include/clang/Basic/Specifiers.h:31
+ // this flag is used in combination with others.
+ HAS_EXPR = 1 << 2
+ };
----------------
Instead of exposing this serialization detail here, please localize this to the serialization code. One nice way is to serialize `(Kind << 1) | HasExpr` (use the bottom bit for the flag, not the top bit).
================
Comment at: clang/include/clang/Sema/DeclSpec.h:437
+ Friend_specified(false), Constexpr_specified(false),
+ FS_explicit_specifier(nullptr, ExplicitSpecFlag::RESOLVED_FALSE),
+ Attrs(attrFactory), writtenBS(), ObjCQualifiers(nullptr) {}
----------------
Default-construct this.
================
Comment at: clang/include/clang/Sema/DeclSpec.h:572-574
+ return FS_explicit_specifier.getFlag() !=
+ ExplicitSpecFlag::RESOLVED_FALSE ||
+ FS_explicit_specifier.getExpr();
----------------
Can you use `FS_explicit_specifier.isSpecified()` here?
================
Comment at: clang/include/clang/Sema/DeclSpec.h:593-594
FS_virtualLoc = SourceLocation();
- FS_explicit_specified = false;
+ FS_explicit_specifier =
+ ExplicitSpecifier(nullptr, ExplicitSpecFlag::RESOLVED_FALSE);
FS_explicitLoc = SourceLocation();
----------------
Can you use `= ExplicitSpecifier();`?
================
Comment at: clang/include/clang/Sema/Sema.h:10124
+ /// tryResolveExplicitSpecifier - Attempt to resolve the explict specifier.
+ /// Returns true if the explicit specifier is now.
+ bool tryResolveExplicitSpecifier(ExplicitSpecifier &ExplicitSpec);
----------------
Missing last word from this comment?
================
Comment at: clang/include/clang/Serialization/ASTBitCodes.h:1444-1449
+ /// A CXXConstructorDecl record with storage for an ExplicitSpecifier.
+ DECL_CXX_EXPLICIT_CONSTRUCTOR,
+
+ /// A CXXConstructorDecl record for an inherited constructor with storage
+ /// for an ExplicitSpecifier.
+ DECL_CXX_EXPLICIT_INHERITED_CONSTRUCTOR,
----------------
For previous similar cases we've put the necessary data to pass into `::CreateDeserialized(...)` at the start of the record (eg, see the `Record.readInt()` calls in `ReadDeclRecord`). We're starting to get a combinatorial explosion here (all 4 combinations of the two possible trailing objects), so maybe now's the time for that.
================
Comment at: clang/include/clang/Serialization/ASTReader.h:2434
+ ExplicitSpecifier readExplicitSpec() {
+ uint64_t flag = readInt();
+ if (flag & static_cast<uint64_t>(ExplicitSpecFlag::HAS_EXPR)) {
----------------
For consistency with nearby code, please name this variable starting with a capital letter.
================
Comment at: clang/lib/AST/DeclCXX.cpp:1905
+ C, nullptr, SourceLocation(),
+ ExplicitSpecifier(nullptr, ExplicitSpecFlag::RESOLVED_FALSE),
+ DeclarationNameInfo(), QualType(), nullptr, SourceLocation());
----------------
Default-construct.
================
Comment at: clang/lib/AST/DeclCXX.cpp:2367
C, nullptr, SourceLocation(), DeclarationNameInfo(), QualType(), nullptr,
- false, false, false, false, InheritedConstructor());
+ ExplicitSpecifier(nullptr, ExplicitSpecFlag::RESOLVED_FALSE), false,
+ false, false, InheritedConstructor());
----------------
Default-construct.
================
Comment at: clang/lib/AST/DeclCXX.cpp:2370
Result->setInheritingConstructor(Inherited);
+ Result->CXXConstructorDeclBits.hasTrailExplicit = hasTailExplicit;
return Result;
----------------
This leaves the tail-allocated `ExplicitSpecifier` uninitialized, because the constructor thought there was no explicit specifier; consider calling `setExplicitSpecifier(ExplicitSpecifier());` here if `hasTailExplicit`.
================
Comment at: clang/lib/AST/DeclCXX.cpp:2540
+ C, nullptr, SourceLocation(), DeclarationNameInfo(), QualType(), nullptr,
+ false, ExplicitSpecifier(nullptr, ExplicitSpecFlag::RESOLVED_FALSE),
+ false, SourceLocation());
----------------
Default-construct.
================
Comment at: clang/lib/Parse/ParseDecl.cpp:3532
+ if (ExplicitExpr.isUsable()) {
+ CloseParenLoc = ConsumeParen();
+ ExplicitSpec = Actions.ActOnExplicitBoolSpecifier(
----------------
There's no guarantee that you have an `r_paren` here. Use `Tracker.consumeClose()` instead; it will do the right thing (including producing a suitable error if the next token is something other than a `)`).
================
Comment at: clang/lib/Parse/ParseDecl.cpp:3536-3539
+ Tracker.skipToEnd();
+ // TODO: improve parsing recovery by skipping declaration or
+ // backtraking.
+ return;
----------------
Now we have a representation for an invalid *explicit-specifier*, I think you should be able to just carry on here and parse more specifiers and the rest of the declaration.
================
Comment at: clang/lib/Parse/ParseDecl.cpp:3898-3908
Diag(Tok, DiagID)
<< PrevSpec << FixItHint::CreateRemoval(Tok.getLocation());
else if (DiagID == diag::err_opencl_unknown_type_specifier) {
Diag(Tok, DiagID) << getLangOpts().OpenCLCPlusPlus
<< getLangOpts().getOpenCLVersionTuple().getAsString()
<< PrevSpec << isStorageClass;
} else
----------------
In the `isAlreadyConsumed` case, `Tok` will be the wrong token here (it'll be the token after the //explicit-specifier//). Factoring this out into a lambda taking a `SourceLocation` (to be called from the `explicit` case) would help; alternatively you could grab the `SourceLocation` of `Tok` before the `switch` and use it here instead of `Tok`.
================
Comment at: clang/lib/Sema/DeclSpec.cpp:958
+ ? diag::err_duplicate_declspec
+ : diag::warn_duplicate_declspec;
PrevSpec = "explicit";
----------------
The latter case should be `ext_` not `warn_`: under CWG issue 1830 (http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#1830) repeated //decl-specifier//s (including `explicit`) are ill-formed (and that issue is in DR status, so should be applied retroactively).
================
Comment at: clang/lib/Sema/DeclSpec.cpp:1323
+ FS_explicit_specifier =
+ ExplicitSpecifier(nullptr, ExplicitSpecFlag::RESOLVED_FALSE);
FS_virtualLoc = FS_explicitLoc = SourceLocation();
----------------
Default-construct.
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:10880
+
+ExplicitSpecifier Sema::ActOnExplicitBoolSpecifier(SourceLocation Loc,
+ Expr *ExplicitExpr) {
----------------
`Loc` here is unused; can you remove it?
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:11038
+ /*TInfo=*/nullptr,
+ ExplicitSpecifier(nullptr, ExplicitSpecFlag::RESOLVED_FALSE),
+ /*isInline=*/true, /*isImplicitlyDeclared=*/true, Constexpr);
----------------
Default-construct.
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:12611
Context, ClassDecl, ClassLoc, NameInfo, QualType(), /*TInfo=*/nullptr,
- /*isExplicit=*/false, /*isInline=*/true, /*isImplicitlyDeclared=*/true,
- Constexpr);
+ ExplicitSpecifier(nullptr, ExplicitSpecFlag::RESOLVED_FALSE),
+ /*isInline=*/true,
----------------
Default-construct.
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:12742
Context, ClassDecl, ClassLoc, NameInfo, QualType(), /*TInfo=*/nullptr,
- /*isExplicit=*/false, /*isInline=*/true, /*isImplicitlyDeclared=*/true,
- Constexpr);
+ ExplicitSpecifier(nullptr, ExplicitSpecFlag::RESOLVED_FALSE),
+ /*isInline=*/true,
----------------
Default-construct.
================
Comment at: clang/lib/Sema/SemaInit.cpp:3817-3831
+ if (AllowExplicit || !Conv->isExplicit()) {
if (ConvTemplate)
- S.AddTemplateConversionCandidate(ConvTemplate, I.getPair(),
- ActingDC, Initializer, DestType,
- CandidateSet, AllowExplicit,
- /*AllowResultConversion*/false);
+ S.AddTemplateConversionCandidate(
+ ConvTemplate, I.getPair(), ActingDC, Initializer, DestType,
+ CandidateSet, AllowExplicit, AllowExplicit,
+ /*AllowResultConversion*/ false);
else
----------------
We no longer pass `false` for `AllowExplicit` to `Add*Candidate` when `CopyInitializing` is `true`. Is that an intentional change?
================
Comment at: clang/lib/Sema/SemaInit.cpp:3819
+ AllowExplicit = AllowExplicit && !CopyInitializing;
+ if (AllowExplicit || Conv->isMaybeNotExplicit()) {
if (ConvTemplate)
----------------
Tyker wrote:
> rsmith wrote:
> > Consider just removing this `if`. It's not clear that it carries its weight.
> this if prevent conversion that are already known not to be valid from being added to the overload set. removing it is still correct because it will be removed later. but we would be doing "useless" work because we are adding the to the overload set knowing they will be removed.
> the only benefit i see of removing this if is to make all explicit conversion appear in overload resolution failure messages. is it the intent ?
The intent was to simplify the logic here, and to have only one place where we check the same explicit-specifier (rather than checking it twice, with one check sometimes being incomplete). Let's leave this alone for now, to keep this patch smaller.
================
Comment at: clang/lib/Sema/SemaInit.cpp:9361
// Only consider converting constructors.
- if (GD->isExplicit())
+ if (!GD->isMaybeNotExplicit())
continue;
----------------
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.
================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:375
+ return ES;
+ ExplicitSpecifier Result(nullptr, ExplicitSpecFlag::UNRESOLVED);
+ Expr *OldCond = ES.getExpr();
----------------
Please add an `ExplicitSpecifier ExplicitSpecifier::invalid()` static member function to build the "invalid" state, rather than hardcoding the invalid state here.
================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:384
+ if (SubstResult.isInvalid()) {
+ return Result;
+ }
----------------
`return ExplicitSpecifier::invalid();` here, and don't declare `Result` until you are ready to initialize it below.
================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:1754-1757
+ ExplicitSpecifier ExplicitSpec = instantiateExplicitSpecifier(
+ SemaRef, TemplateArgs, DGuide->getExplicitSpecifier(), DGuide);
+ if (ExplicitSpec.isInvalid())
+ return nullptr;
----------------
C++ language rules require that we substitute into the declaration in lexical order (this matters for SFINAE versus non-immediate-context errors). You should instantiate the explicit specifier prior to the call to `SubstFunctionType` to match source order.
================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:2067-2071
+ ExplicitSpecifier ExplicitSpec = instantiateExplicitSpecifier(
+ SemaRef, TemplateArgs, Constructor->getExplicitSpecifier(),
+ Constructor);
+ if (ExplicitSpec.isInvalid())
+ return nullptr;
----------------
Likewise, this should be done after instantiating the template parameter lists and before substitution into the function type.
================
Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:2021
void ASTDeclReader::VisitCXXConversionDecl(CXXConversionDecl *D) {
+ // assert(false && "TODO");
+ D->setExplicitSpecifier(Record.readExplicitSpec());
----------------
Please delete this.
================
Comment at: clang/test/CXX/temp/temp.deduct.guide/p3.cpp:9-10
// We interpret this as also extending to the validity of redeclarations. It's
// a bit of a stretch (OK, a lot of a stretch) but it gives desirable answers.
+A() -> A<int>; // expected-error {{redeclaration of a deduction guide}}
----------------
Please delete this (out of date) comment.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60934/new/
https://reviews.llvm.org/D60934
More information about the cfe-commits
mailing list