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