aaron.ballman added a comment. Thank you for working on this!
> Immediate calls in default arguments and defaults members are not evaluated. Er, I think this meant to say "are not evaluated immediately, unlike what the misnamed term of art would suggest." ;-) > As a result of this patch, unused default member initializers are not > considered odr-used, ... So this is a potentially breaking change because instantiations that used to happen might no longer happen. Do you have test coverage showing that break in behavior? ================ Comment at: clang/docs/ReleaseNotes.rst:507-508 ``-std=gnu++14`` to their build settings to restore the previous behaviour. +- Implemented DR2631. Invalid ``consteval`` calls in default arguments and default + member initializers are diagnosed when and if the default is used. ---------------- Should this be listed as a potentially breaking change as this now 1) potentially changes the value people were getting for `source_location::current()` as a default argument, and 2) potentially causes instantiation differences due to the change in ODR use? ================ Comment at: clang/include/clang/AST/ExprCXX.h:1307-1315 + const Expr *getRewrittenExpr() const { + return hasRewrittenInit() ? *getTrailingObjects<Expr *>() : nullptr; + } + Expr *getRewrittenExpr() { + return hasRewrittenInit() ? *getTrailingObjects<Expr *>() : nullptr; + } + ---------------- Comments on these methods would be appreciated, otherwise we don't know what kind of adjustments you're talking about. ================ Comment at: clang/include/clang/AST/ExprCXX.h:1396-1404 + const Expr *getExpr() const; + Expr *getExpr(); + + const Expr *getRewrittenExpr() const { + return hasRewrittenInit() ? *getTrailingObjects<Expr *>() : nullptr; } + ---------------- Comments here would also be appreciated. ================ Comment at: clang/include/clang/Sema/Sema.h:1336 + // When evaluating immediate functions in the initializer of a default + // parameter or default member initializer, this is is the declaration whose + // default initializer is being evaluated and the location of the call ---------------- ================ Comment at: clang/include/clang/Sema/Sema.h:9618 + + ExpressionEvaluationContextRecord::InitializationContext + InnermostDeclarationWithDelayedImmediateInvocations() const { ---------------- Rather than go with `isValid()` on this class type, why not have this function return an `Optional` instead (so `InitializationContext` can always be assumed to be valid)? ================ Comment at: clang/include/clang/Sema/Sema.h:9633 + + ExpressionEvaluationContextRecord::InitializationContext + OutermostDeclarationWithDelayedImmediateInvocations() const { ---------------- Same here as above. ================ Comment at: clang/lib/AST/ExprCXX.cpp:981-982 +const Expr *CXXDefaultArgExpr::getAdjustedRewrittenExpr() const { + if (!hasRewrittenInit()) + return nullptr; + const Expr *Init = getRewrittenExpr(); ---------------- Should this be an assert instead? ================ Comment at: clang/lib/AST/ExprCXX.cpp:985 + + if (auto *E = dyn_cast_or_null<FullExpr>(Init)) + if (!isa<ConstantExpr>(E)) ---------------- ================ Comment at: clang/lib/AST/ExprCXX.cpp:992-993 +Expr *CXXDefaultArgExpr::getAdjustedRewrittenExpr() { + if (!hasRewrittenInit()) + return nullptr; + Expr *Init = getRewrittenExpr(); ---------------- Same here as above? ================ Comment at: clang/lib/AST/ExprCXX.cpp:995 + Expr *Init = getRewrittenExpr(); + if (auto *E = dyn_cast_or_null<FullExpr>(Init)) + if (!isa<ConstantExpr>(E)) ---------------- ================ Comment at: clang/lib/AST/ExprCXX.cpp:1013-1015 + if (CXXDefaultInitExprBits.HasRewrittenInit) { + *getTrailingObjects<Expr *>() = RewrittenInitExpr; + } ---------------- ================ Comment at: clang/lib/Parse/ParseDeclCXX.cpp:3188 + Actions, + isa_and_nonnull<FieldDecl>(D) + ? Sema::ExpressionEvaluationContext::PotentiallyEvaluatedIfUsed ---------------- ================ Comment at: clang/lib/Sema/SemaExpr.cpp:5895 // blocks in default argument expression can never capture anything. - if (auto Init = dyn_cast<ExprWithCleanups>(Param->getInit())) { + if (auto InitWithCleanup = dyn_cast<ExprWithCleanups>(Init)) { // Set the "needs cleanups" bit regardless of whether there are ---------------- ================ Comment at: clang/lib/Sema/SemaExpr.cpp:5909 + SkipImmediateInvocations; + MarkDeclarationsReferencedInExpr(Init, true); return false; ---------------- ================ Comment at: clang/lib/Sema/SemaExpr.cpp:5916-5919 + if (const FunctionDecl *FD = + dyn_cast_or_null<FunctionDecl>(E->getCalleeDecl())) { + HasImmediateCalls |= FD->isConsteval(); + } ---------------- ================ Comment at: clang/lib/Sema/SemaExpr.cpp:5971-5973 + if (!Init && !Param->hasUnparsedDefaultArg()) { + + // Mark that we are replacing a default argument first. ---------------- ================ Comment at: clang/lib/Sema/SemaExpr.cpp:5974-5975 + // Mark that we are replacing a default argument first. + // If we are instanciating a template we won't have to + // retransforme immediate calls + EnterExpressionEvaluationContext EvalContext( ---------------- ================ Comment at: clang/lib/Sema/SemaExpr.cpp:6022 + + CXXRecordDecl *ParentRD = cast<CXXRecordDecl>(Field->getParent()); + ExpressionEvaluationContextRecord::InitializationContext ---------------- ================ Comment at: clang/lib/Sema/SemaExpr.cpp:6043-6048 + for (auto *L : Lookup) { + if (isa<FieldDecl>(L)) { + Pattern = cast<FieldDecl>(L); + break; + } + } ---------------- ================ Comment at: clang/lib/Sema/SemaExpr.cpp:6050-6059 + if (!Pattern->hasInClassInitializer()) { + Field->setInvalidDecl(); + return ExprError(); + } + if (InstantiateInClassInitializer(Loc, Field, Pattern, + getTemplateInstantiationArgs(Field))) { + // Don't diagnose this again. ---------------- ================ Comment at: clang/lib/Sema/SemaExpr.cpp:18821 - // If the variable is declared in the current context, there is no need to ---------------- Spurious whitespace change. ================ Comment at: clang/test/CXX/class/class.local/p1-0x.cpp:14 int& x2 = x; // expected-error{{reference to local variable 'x' declared in enclosing lambda expression}} - }; + }c; // expected-note {{required here}} }; ---------------- Double-checking: you did intend to name that local variable, right? ================ Comment at: clang/test/CodeGenCXX/default-arguments-with-immediate.cpp:4 + +consteval int immediate() {return 0;} +static int ext(); ---------------- ================ Comment at: clang/test/CodeGenCXX/default-arguments-with-immediate.cpp:22 + +// CHECK: define {{.*}} i32 @_ZL3extv() + ---------------- Move this check line up to where ext is declared? ================ Comment at: clang/test/SemaCXX/cxx2a-consteval-default-params.cpp:5 + +consteval int undefined(); // expected-note 4{{declared here}} + ---------------- ================ Comment at: clang/test/SemaCXX/cxx2a-consteval-default-params.cpp:29-30 + return defaulted; + }() // expected-note {{in the default initalizer of 'defaulted'}} +) { + return 0; ---------------- I think it'd be useful to show that use in an unevaluated context still works. ================ Comment at: clang/test/SemaCXX/cxx2a-consteval-default-params.cpp:41-43 + int b = [](int no_error = undefined()) { + return no_error; + }(); ---------------- Comments here explaining why this is fine would be appreciated. ================ Comment at: clang/test/SemaCXX/cxx2a-consteval-default-params.cpp:52 + }(); // expected-note {{in the default initalizer of 'error'}} +} i; // expected-note {{in implicit default constructor}} ---------------- Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136554/new/ https://reviews.llvm.org/D136554 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits