rsmith marked an inline comment as done.
rsmith added inline comments.
================
Comment at: clang/include/clang/AST/DeclCXX.h:2022
+
+ bool isEquivalentExplicit(const CXXDeductionGuideDecl *Other) const;
+ bool isExplicit() const {
----------------
I think this is too special-case to live on `CXXDeductionGuideDecl`. Instead, I
suggest:
* Add a class wrapping `ExplicitSpecInfo`, maybe called `ExplicitSpecifier`
* Change all the AST interfaces that deal in `ExplicitSpecInfo` to instead
deal in `ExplicitSpecifier` objects
* Move this comparison logic there, along with the more uncommon members you
add below.
I think we should be able to reduce the set of `explicit`-related members on
the various `Decl` subclasses to just `getExplicitSpecifier()` and
`isExplicit()`.
================
Comment at: clang/include/clang/AST/DeclCXX.h:2033
+
+ void setExplicitSpecifier(ExplicitSpecInfo ESI);
+
----------------
Generally we don't want to have setters in the AST; the AST is intended to be
immutable after creation. Is this necessary?
================
Comment at: clang/include/clang/AST/DeclCXX.h:2519
+ ExplicitSpecInfo ExplicitSpecifier;
+
----------------
Consider storing this in a `TrailingObject` rather than making all constructors
a pointer wider for this (presumably relatively uncommon) special case.
================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:37
+def warn_explicit_bool_breaking_change_cxx17 : Warning<
+ "this expression would be parsed as explicit(bool) in C++2a">
+ , InGroup<CXX2aCompat>, DefaultIgnore;
----------------
would be -> will be
================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:38
+ "this expression would be parsed as explicit(bool) in C++2a">
+ , InGroup<CXX2aCompat>, DefaultIgnore;
+def note_explicit_bool_breaking_change_cxx2a : Note<
----------------
Nit: comma on previous line, please.
================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:40
+def note_explicit_bool_breaking_change_cxx2a : Note<
+ "this expression is parsed as explicit(bool) since C++2a">;
+
----------------
This should be a warning in the `CXXPre2aCompat` group, phrased as
"explicit(bool) is incompatible with C++ standards before C++2a".
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2124-2129
+def err_deduction_guide_explicit_bool_mismatch : Error<
+ "the deduction guide is %select{not ||potentially }0explicit but "
+ "previous declaration was %select{not ||potentially }1explicit">;
+def err_unresolved_explicit_mismatch : Error<
+ "explicit specifier is not equivalent to the explicit specifier"
+ " from previous declaration">;
----------------
I don't believe there is any such rule; instead, the rule is simply that
deduction guides cannot be redeclared at all. See [temp.deduct.guide]/3: "Two
deduction guide declarations in the same translation unit for the same class
template shall not have equivalent parameter-declaration-clauses." (That's
obviously wrong; we should compare the template-head as well, but the intention
is that deduction guides cannot be redeclared.)
================
Comment at: clang/include/clang/Basic/Specifiers.h:27-29
+ ESF_resolved_false,
+ ESF_resolved_true,
+ ESF_unresolved
----------------
Please capitalize these enumerator names, and consider using an 'enum class'
instead of an `ESF_` prefix. (We use lowercase for the next few `enum`s because
their enumerators are (prefixed) keywords.)
================
Comment at: clang/include/clang/Sema/DeclSpec.h:376
+ using ExplicitSpecifierInfo =
+ llvm::PointerIntPair<Expr *, 2, ExplicitSpecFlag>;
+
----------------
If this is a fully-semantically-analyzed explicit-specifier, this should use
the same type we use in the AST representation. If it's a
parsed-but-not-yet-semantically-analyzed explicit-specifier, using
`ExplicitSpecFlag` doesn't seem right: you haven't tried to resolve it yet in
that case.
================
Comment at: clang/lib/AST/DeclCXX.cpp:1867-1878
+static bool MaybeResolveExplicit(FunctionDecl *Decl, ExplicitSpecInfo &ESI) {
+ if (ESI.getInt() != ESF_unresolved || !ESI.getPointer())
+ return true;
+ APValue Result;
+ if (ESI.getPointer()->EvaluateWithSubstitution(
+ Result, Decl->getASTContext(), Decl, llvm::ArrayRef<Expr *>())) {
+ ESI.setInt(Result.getInt().getBoolValue() ? ESF_resolved_true
----------------
Please don't evaluate the explicit specifier here. Instead, it should be
evaluated by `Sema` when it's initially formed (if it's not value-dependent)
and during template instantiation (if a value-dependent explicit-specifier
becomes non-value-dependent). Note that `Sema` needs to evaluate it at those
times anyway, in order to properly diagnose non-constant explicit-specifiers.
================
Comment at: clang/lib/AST/DeclCXX.cpp:2514-2522
bool CXXConstructorDecl::isConvertingConstructor(bool AllowExplicit) const {
// C++ [class.conv.ctor]p1:
// A constructor declared without the function-specifier explicit
// that can be called with a single parameter specifies a
// conversion from the type of its first parameter to the type of
// its class. Such a constructor is called a converting
// constructor.
----------------
I don't think this should work this way any more: we can't tell whether a
constructor template forms a converting constructor before substituting into it.
Another point of note: the "that can be called with a single parameter" part of
the language rule has been removed (and was never necessary). So
"isConvertingConstructor" is really just "is not explicit".
So: I think we should rename this function to a more general "could this
function ever be called with exactly N arguments?" function (moved to
`FunctionDecl`, adjacent to `getMinRequiredArguments`), and use it in the
current callers of `isConvertingConstructor` just to filter out functions that
can't be called with exactly one argument from the candidate list for
copy-initialization. Then change the current callers of this function to call
`Add*OverloadCandidate` with `AllowExplicit = false` in the cases where they
currently require a converting constructor.
It'd make sense to do that as a separate change / refactoring before adding
`explicit(bool)` support.
================
Comment at: clang/lib/AST/DeclPrinter.cpp:555
+static void ExplicitSpecInfoPrinter(ExplicitSpecInfo ESI,
+ llvm::raw_ostream &Out,
----------------
Function names should generally be verb phrases, not noun phrases.
`printExceptionSpecifier` maybe?
================
Comment at: clang/lib/Parse/ParseDecl.cpp:3523
+ ExplicitSpecFlag ExplicitFlag = ESF_resolved_true;
+ if (GetLookAheadToken(1).is(tok::l_paren)) {
+ if (getLangOpts().CPlusPlus2a) {
----------------
Please try to avoid using lookahead for this. (You may need to change the code
after the loop so you can tell it that you already consumed the specifier, or
factor out that code into a lambda that you can call from both there and here.)
================
Comment at: clang/lib/Parse/ParseDecl.cpp:3533-3537
+ Diag(ParenLoc, diag::note_explicit_bool_breaking_change_cxx2a)
+ << FixItHint::CreateReplacement(
+ SourceRange(ExplicitLoc, ExplicitLoc.getLocWithOffset(
+ /*explicit*/ 8)),
+ "explicit(true)");
----------------
Rakete1111 wrote:
> This is a useful diagnostic but you'll need to fix (a lot) of false positives:
>
> ```
> template <typename T> struct Foo { explicit(T{} +) Foo(); };
> ```
>
> gets me:
>
> ```
> main.cpp:1:50: error: expected expression
> template <typename T> struct Foo { explicit(T{} +) Foo(); };
> ^
> main.cpp:1:44: note: this expression is parsed as explicit(bool) since C++2a
> template <typename T> struct Foo { explicit(T{} +) Foo(); };
> ~~~~~~~~^
> explicit(true)
> ```
>
> Fixit hints should only be used when it is 100% the right fix that works,
> always.
This "add a note after an error" construct is an anti-pattern, and will fail in
a bunch of cases (for example: if multiple diagnostics are produced, it gets
attached to the last one rather than the first; if a disabled warning / remark
is produced last, the note is not displayed at all).
As noted above, the conventional way to handle this is to unconditionally
produce a `-Wc++17-compat` warning when we see `explicit(`. Attaching a context
note to the diagnostic if building the expression fails (catching both `Parse`
and `Sema` diagnostics) will require more invasive changes and I'd prefer to
see that left to a separate patch (if we do it at all).
================
Comment at: clang/lib/Sema/DeclSpec.cpp:959
+ // Each decl-specifier shall appear at most once in a complete
+ // decl-specifier-seq, except that long may appear twice.
+ if (hasExplicitSpecifier()) {
----------------
Tyker wrote:
> Quuxplusone wrote:
> > Spelling/grammar/capitalization-of-C++2a.
> > Also, it seems to me that you've got a CWG wording issue here: what does
> > N4810 mean by "Each //decl-specifier// shall appear at most once in a
> > complete //decl-specifier-seq//, except that `long` may appear twice"? What
> > is "each" decl-specifier? Is `explicit(true)` a different decl-specifier
> > from `explicit(1+1==2)`? Is `explicit(true)` different from
> > `explicit(false)`?
> I agree that it isn't clear. but i assumed that each counts every
> explicit-specifier as one.
I reported this on the core reflector nearly a year ago
(http://lists.isocpp.org/core/2018/06/4673.php); sadly the issue process is a
long way behind so we don't have an issue number yet, but I think the intent is
clear, and matches what is implemented here.
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:10866
+ExprResult Sema::ActOnExplicitBoolSpecifier(SourceLocation Loc,
+ ExplicitSpecFlag &Flag,
----------------
Return an `ExplicitSpecifier` here rather than splitting it up into an
`ExprResult` and an `ExplicitSpecFlag`.
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:10870
+ ExprResult Converted =
+ CheckBooleanCondition(Loc, ExplicitExpr, /*isConstexpr=*/true);
+ if (Converted.isInvalid())
----------------
Rakete1111 wrote:
> This assumes that we are in a [constexpr] if, leading to errors like this:
>
> ```
> int a;
> template <typename T> struct Foo { explicit(a) Foo(); };
> ```
>
> ```
> main.cpp:2:45: error: constexpr if condition is not a constant expression
> template <typename T> struct Foo { explicit(a) Foo(); };
> ^
> ```
This is wrong; a "condition" is the thing that appears after `if (` or `while
(` or in the middle term of a `for (;;`.
An explicit-specifier requires a contextually-converted constant expression of
type `bool`, so you should call `CheckConvertedContantExpression`, and add a
new `CCEKind` for this conversion (look at what we do for `CCEK_ConstexprIf` to
find how to get it to convert to `bool`). You'll need to update its
`CCEKind`-specific diagnostics (eg, `ext_cce_narrowing`, `err_expr_not_cce`) to
properly diagnose explicit-specifiers.
================
Comment at: clang/lib/Sema/SemaInit.cpp:3818
- if ((AllowExplicit && !CopyInitializing) || !Conv->isExplicit()) {
+ AllowExplicit = AllowExplicit && !CopyInitializing;
+ if (AllowExplicit || Conv->isMaybeNotExplicit()) {
----------------
Please use a variable with a different name here, such as
`AllowExplicitConversion`. Changing a variable with a larger scope to match the
semantics required in a smaller scope like this is error-prone (even though the
code is correct right now). Also, please hoist that new variable out of the
`for` loop.
================
Comment at: clang/lib/Sema/SemaInit.cpp:3819
+ AllowExplicit = AllowExplicit && !CopyInitializing;
+ if (AllowExplicit || Conv->isMaybeNotExplicit()) {
if (ConvTemplate)
----------------
Consider just removing this `if`. It's not clear that it carries its weight.
================
Comment at: clang/lib/Sema/SemaInit.cpp:5001-5002
+
if (!Info.Constructor->isInvalidDecl() &&
Info.Constructor->isConvertingConstructor(AllowExplicit)) {
if (Info.ConstructorTmpl)
----------------
Likewise.
================
Comment at: clang/lib/Sema/SemaInit.cpp:9361
// Only consider converting constructors.
- if (GD->isExplicit())
+ if (!GD->isMaybeNotExplicit())
continue;
----------------
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`.
================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:387-403
+ if (!Cond->isTypeDependent()) {
+ ExprResult Converted = S.PerformContextuallyConvertToBool(Cond);
+ if (Converted.isInvalid())
+ return Result;
+ Cond = Converted.get();
+ }
+ SmallVector<PartialDiagnosticAt, 8> Diags;
----------------
Please factor out the common code between this and `ActOnExplicitSpecifier`.
(We usually call such a function `BuildSomething`, such as
`Sema::BuildExplicitSpecifier`).
================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:388
+ if (!Cond->isTypeDependent()) {
+ ExprResult Converted = S.PerformContextuallyConvertToBool(Cond);
+ if (Converted.isInvalid())
----------------
This needs to be done in a `ConstantEvaluated` context too.
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
