[PATCH] D136554: Implement CWG2631
Aaron Ballman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Oct 31 06:11:52 PDT 2022
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
More information about the cfe-commits
mailing list