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
