[PATCH] D60934: [clang] adding explicit(bool) from c++2a
Richard Smith - zygoloid via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Apr 25 11:52:07 PDT 2019
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
More information about the cfe-commits
mailing list