[PATCH] D36915: [Sema] Diagnose local variables and parameters captured by lambda and block expressions in a default argument
rsmith added a comment.
Please take a look at
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0588r1.html, which
respecifies the rules for lambda capture and its interaction with default
arguments, and has been voted into the C++ working paper as a defect report
resolution. The approach there is to use a purely syntactic, scope-based
mechanism to detect problems such as this. (In dependent contexts, we can track
on the `DeclRefExpr` whether the name is odr-usable, in case we can't tell
whether it's odr-used from the template definition alone.)
Comment at: include/clang/Sema/Sema.h:1062
+
+ const DeclContext *ParentOfDefaultArg = nullptr;
+
There are lots of cases where we switch context in the middle of handling an
expression, for instance to instantiate a template (or even *parse* a template
in MSVC-compatible delayed template parsing mode). It's not reasonable to add a
new form of state that all those places will need to save and restore
themselves.
Please consider whether this would make sense as a member of the
`ExpressionEvaluationContextRecord` or similar.
Comment at: lib/Sema/SemaExpr.cpp:4520-4541
+ // Add mappings for instantiated parameters appearing before Param. This
+ // is needed to instantiate default argument expressions referencing
+ // other parameters in unevaluated contexts.
+ if (FunctionDecl *Pattern = FD->getTemplateInstantiationPattern()) {
+auto I = FD->param_begin();
+for (const auto *PVD : Pattern->parameters()) {
+ if (*I == Param)
Use `addInstantiatedParametersToScope` for this.
This bugfix looks to be independent of the fix for lambdas; can you factor it
out into a separate patch?
https://reviews.llvm.org/D36915
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36915: [Sema] Diagnose local variables and parameters captured by lambda and block expressions in a default argument
aaron.ballman added inline comments.
Comment at: lib/Sema/Sema.cpp:1677-1684
+Sema::DefaultArgRAII::DefaultArgRAII(Sema )
+: Actions(S), PrevParent(S.getParentOfDefaultArg()) {
+ S.setParentOfDefaultArg(S.CurContext);
+}
+
+Sema::DefaultArgRAII::~DefaultArgRAII() {
+ Actions.setParentOfDefaultArg(PrevParent);
Any reason not to inline this in the header file?
Comment at: lib/Sema/SemaExpr.cpp:4523
+ // other parameters in unevaluated contexts.
+ if (FunctionDecl *Pattern = FD->getTemplateInstantiationPattern()) {
+auto I = FD->param_begin();
`const FunctionDecl *`
Comment at: lib/Sema/SemaExpr.cpp:4535
+ unsigned NumExpansions =
+ *getNumArgumentsInExpansion(PVD->getType(), MutiLevelArgList);
+ CurrentInstantiationScope->MakeInstantiatedLocalArgPack(PVD);
Is it possible for the `Optional<>` returned here to not hold a value?
Comment at: lib/Sema/SemaExpr.cpp:13746-13751
+unsigned DiagID;
+if (isa(var))
+ DiagID = diag::err_param_default_argument_references_param;
+else
+ DiagID = diag::err_param_default_argument_references_local;
+S.Diag(loc, DiagID) << var->getDeclName();
Ternary operator in `S.Diag()` would be just as clear, I think.
Also, you should not need to call `getDeclName()` as the diagnostic engine will
automatically do the right thing. As is stands, this won't properly quote the
name in the diagnostic.
https://reviews.llvm.org/D36915
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36915: [Sema] Diagnose local variables and parameters captured by lambda and block expressions in a default argument
rjmccall added a comment. This looks good to me. https://reviews.llvm.org/D36915 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36915: [Sema] Diagnose local variables and parameters captured by lambda and block expressions in a default argument
ahatanak added a comment. ping https://reviews.llvm.org/D36915 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36915: [Sema] Diagnose local variables and parameters captured by lambda and block expressions in a default argument
ahatanak added a comment. ping. Any comments on this patch or alternate approaches? https://reviews.llvm.org/D36915 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36915: [Sema] Diagnose local variables and parameters captured by lambda and block expressions in a default argument
ahatanak added a comment. ping https://reviews.llvm.org/D36915 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36915: [Sema] Diagnose local variables and parameters captured by lambda and block expressions in a default argument
ahatanak updated this revision to Diff 114753.
ahatanak added a comment.
Address review comments.
- Detect invalid references to parameters or local variables by default
arguments in tryCaptureVariable. Before parsing or instantiating the default
argument expression, the enclosing DeclContext is saved to ParentOfDefaultArg,
which tryCaptureVariable uses to detect invalid references (if the referenced
variable belongs to ParentOfDefaultArg or an enclosing DeclContext, it is not
valid).
- In CheckCXXDefaultArgExpr, save the parameters and their instantiations that
appear before the parameter with default argument to the current
LocalInstantiationScope so that findInstantiationOf doesn't assert when it
tries to find the instantiation of a parameter that is referenced in the
default arugment.
There are still cases where clang rejects references to local variables or
parameters that shouldn't be rejected. For example:
- Local variables or parameters referenced in _Generic's controlling-expression
or the expressions of the selections that are not chosen.
- The following code is rejected even though 'x' is not odr-used:
void func() {
const int x = 1;
void foo1(int a0 = x);
}
- dcl.fct.default/p7.cpp
I plan to work on a fix after this patch is committed.
https://reviews.llvm.org/D36915
Files:
include/clang/Sema/Sema.h
lib/Parse/ParseDecl.cpp
lib/Sema/Sema.cpp
lib/Sema/SemaExpr.cpp
test/CXX/dcl.decl/dcl.meaning/dcl.fct.default/p7.cpp
test/CXX/expr/expr.prim/expr.prim.lambda/p13.cpp
test/SemaCXX/default1.cpp
test/SemaObjCXX/blocks.mm
Index: test/SemaObjCXX/blocks.mm
===
--- test/SemaObjCXX/blocks.mm
+++ test/SemaObjCXX/blocks.mm
@@ -169,3 +169,17 @@
return b; // expected-error {{no viable conversion from returned value of type 'MoveBlockVariable::B0' to function return type 'MoveBlockVariable::B1'}}
}
}
+
+namespace DefaultArg {
+void test() {
+ id x;
+ void func0(id a0, id a1 = ^{ (void) }); // expected-error {{default argument references parameter 'a0'}}
+ void func1(id a0, id a1 = ^{ (void) }); // expected-error {{default argument references local variable 'x' of enclosing function}}
+ void func2(id a0, id a1 = ^{ (void)sizeof(a0); });
+ void func3(id a0 = ^{ (void)sizeof(x); });
+ void func4(id a0, id a1 = ^{
+^{ (void) }(); // expected-error {{default argument references parameter 'a0'}}
+[=](){ (void) }(); // expected-error {{default argument references parameter 'a0'}}
+ });
+}
+}
Index: test/SemaCXX/default1.cpp
===
--- test/SemaCXX/default1.cpp
+++ test/SemaCXX/default1.cpp
@@ -78,3 +78,79 @@
void PR20769_b(int = 1);
void PR20769_b() { void PR20769_b(int = 2); }
+
+#if __cplusplus >= 201103L // C++11 or later
+struct S2 {
+ template
+ S2(T&&) {}
+};
+
+template
+void func0(int a0, S2 a1 = [](){ (void) }); // expected-error {{default argument references parameter 'a0'}}
+
+// FIXME: There shouldn't be any warnings about variables referenced in the
+//controlling-expression or the expressions of the selections that are
+//not chosen.
+template
+void func1(T a0, int a1, S2 a2 = _Generic((a0), default: [](){ (void) }, int: 0)); // expected-error {{default argument references parameter 'a0'}}
+
+template
+void func2(S2 a0 = [](){
+ int t; [](){ (void)}();
+});
+
+template
+void func3(int a0, S2 a1 = [](){
+ [=](){ (void)}(); // expected-error {{default argument references parameter 'a0'}}
+});
+
+void func4(int, int);
+
+template
+void func5(Ts...a0, S2 a1 = [](){ func4(a0...); }) { // expected-error 2 {{default argument references parameter 'a0'}}
+}
+
+template
+void func6(Ts...a0, S2 a1 = [](){ (void)sizeof...(a0); }) {
+}
+
+double d;
+
+void test1() {
+ int i;
+ float f;
+ // FIXME: There shouldn't be any warnings about variables referenced in the
+ //controlling-expression or the expressions of the selections that are
+ //not chosen.
+ void foo0(int a0 = _Generic((f), double: d, float: f)); // expected-error 2 {{default argument references local variable 'f' of enclosing function}}
+ void foo1(int a0 = _Generic((d), double: d, float: f)); // expected-error {{default argument references local variable 'f' of enclosing function}}
+ void foo2(int a0 = _Generic((i), int: d, float: f)); // expected-error {{default argument references local variable 'i' of enclosing function}} expected-error {{default argument references local variable 'f' of enclosing function}}
+ void foo3(int a0 = _Generic((i), default: d, float: f)); // expected-error {{default argument references local variable 'i' of enclosing function}} expected-error {{default argument references local variable 'f' of enclosing function}}
+
+ void foo4(S2 a0 = [&](){ (void) }); // expected-error {{default argument references local variable 'i' of enclosing function}}
+ void foo5(S2 a0 = [](){
+// No
Re: [PATCH] D36915: [Sema] Diagnose local variables and parameters captured by lambda and block expressions in a default argument
On 28 August 2017 at 13:47, Akira Hatanaka via Phabricator via cfe-commits < cfe-commits@lists.llvm.org> wrote: > clang currently rejects "void foo(int = a);" and so does gcc. > > I'm trying to search the defect reports, but it looks like the c++ > standard's site is currently unreachable. It's http://lists.isocpp.org/core/2017/04/2108.php, but that's too recent for the latest issues list ( http://wiki.edg.com/pub/Wg21albuquerque/CoreWorkingGroup/cwg_active.html). (Apologies for the committee-internal links; as you say, the public site is down.) ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36915: [Sema] Diagnose local variables and parameters captured by lambda and block expressions in a default argument
ahatanak added a comment. clang currently rejects "void foo(int = a);" and so does gcc. I'm trying to search the defect reports, but it looks like the c++ standard's site is currently unreachable. https://reviews.llvm.org/D36915 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36915: [Sema] Diagnose local variables and parameters captured by lambda and block expressions in a default argument
faisalv added a comment.
In https://reviews.llvm.org/D36915#854317, @ahatanak wrote:
> In https://reviews.llvm.org/D36915#849622, @faisalv wrote:
>
> > I don't think this approach is entirely correct for at least the following
> > reasons:
> >
> > 1. in the lambda case the machinery that diagnoses capture failures should
> > be the machinery erroring on the lambda (when the parameter is odr-used)
>
>
> Does this mean that it is OK to have a parameter or local variable appear in
> a potentially-evaluated expression as long as it is not odr-used? I think
> this is slightly different from the standard, which says a parameter or local
> variable cannot appear as a potentially-evaluated expression in a default
> argument.
>
> For example:
>
> void foo2(int);
>
> void func() {
> const int a = 1;
> void foo1(S s = [](){ foo2(a); }); // "a" is not in an unevaluated
> context and is not odr-used.
> }
>
>
>
> I think I need clarification as it will affect how or where we detect this
> error.
'a' is not captured (and does not need to be captured) above, so I think that
code should be ok.
But I also think the following code should be ok at local scope within func.
void foo(int = a);
I thought there was a DR against the standard-draft with the intent of making
these valid (if they are not already so)?
https://reviews.llvm.org/D36915
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36915: [Sema] Diagnose local variables and parameters captured by lambda and block expressions in a default argument
ahatanak added a comment.
In https://reviews.llvm.org/D36915#849622, @faisalv wrote:
> I don't think this approach is entirely correct for at least the following
> reasons:
>
> 1. in the lambda case the machinery that diagnoses capture failures should be
> the machinery erroring on the lambda (when the parameter is odr-used)
Does this mean that it is OK to have a parameter or local variable appear in a
potentially-evaluated expression as long as it is not odr-used? I think this is
slightly different from the standard, which says a parameter or local variable
cannot appear as a potentially-evaluated expression in a default argument.
For example:
void foo2(int);
void func() {
const int a = 1;
void foo1(S s = [](){ foo2(a); }); // "a" is not in an unevaluated context
and is not odr-used.
}
I think I need clarification as it will affect how or where we detect this
error.
https://reviews.llvm.org/D36915
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36915: [Sema] Diagnose local variables and parameters captured by lambda and block expressions in a default argument
ahatanak added a comment.
In https://reviews.llvm.org/D36915#849622, @faisalv wrote:
> I don't think this approach is entirely correct for at least the following
> reasons:
>
> 1. in the lambda case the machinery that diagnoses capture failures should be
> the machinery erroring on the lambda (when the parameter is odr-used)
> 2. in the unevaluated case, once you disable the error, the instantiation
> machinery will fail to find the corresponding instantiated parmvardecl.
Oh right, it stills assert in the unevaluated case. I should have a test case
for that too.
I also found that the following code, which has a lambda that doesn't capture
anything, asserts (in IRGen):
struct S {
template
S(T&&) {}
};
template
void foo1() {
struct S2 {
void foo2(S s = [](){}) {
}
};
S2 s2;
s2.foo2();
}
void foo3() {
foo1();
}
https://reviews.llvm.org/D36915
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36915: [Sema] Diagnose local variables and parameters captured by lambda and block expressions in a default argument
faisalv added a comment. I don't think this approach is entirely correct for at least the following reasons: 1. in the lambda case the machinery that diagnoses capture failures should be the machinery erroring on the lambda (when the parameter is odr-used) 2. in the unevaluated case, once you disable the error, the instantiation machinery will fail to find the corresponding instantiated parmvardecl. I think - in addition to allowing unevaluated uses of parmvardecls by tweaking the DefaultArgumentChecker, you would also need to add the instantiated mappings of the parameters, prior to instantiating the default argument (to avoid the assertion) and perhaps need to tweak DoMarkVarDeclReferenced and/or tryCaptureVariable to make sure such cases for lambdas produce errors (if they don't, w the above fix). Thanks for working on this! https://reviews.llvm.org/D36915 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36915: [Sema] Diagnose local variables and parameters captured by lambda and block expressions in a default argument
ahatanak created this revision.
This patch fixes an assertion failure that occurs when compiling the following
invalid code:
struct S {
template
S(T &&) {}
};
template
void foo1(int a0, S a1 = [](){ (void) } ) { // a0 cannot be used in the
default argument for a1
}
void foo2() {
foo1(1);
}
$ clang++ -std=c++14 -c -o /dev/null test.cpp
Assertion failed: (isa(D) && "declaration not instantiated in this
scope"), function findInstantiationOf, file
llvm/tools/clang/lib/Sema/SemaTemplateInstantiate.cpp, line 2911.
The assertion fails when findInstantiationOf is called to find the instantiated
decl of a0 when instantiating the lambda expression that is the default
argument for a1.
To prevent the assertion failure, this patch makes CheckDefaultArgumentVisitor
visit all subexpressions belonging to a default argument expression and detect
local variables and parameters (that are external to the default argument)
referenced in the default argument expression after the template definition is
parsed. This patch also removes the diagnostic that is printed in test p7.cpp
when a local variable is referenced inside a unevaluated default argument
expression, which I think conforms to c++14 or later.
Also, with this patch, clang prints diagnostics when local variables or
parameters are referenced inside a block expression that is used as a default
argument. I wasn't 100% sure it is legal to use blocks for default arguments (I
found that compiling the code below causes clang to segfault), but it seems to
me that we want to handle blocks in default arguments the same way we handle
lambdas.
void logRange(id i = [](){}) {
}
void foo1() {
logRange();
}
$ clang++ -std=c++14 -c -o /dev/null -fobjc-arc test.mm
rdar://problem/33239958
https://reviews.llvm.org/D36915
Files:
lib/Sema/SemaDeclCXX.cpp
test/CXX/dcl.decl/dcl.meaning/dcl.fct.default/p7.cpp
test/SemaCXX/default1.cpp
test/SemaObjCXX/blocks.mm
Index: test/SemaObjCXX/blocks.mm
===
--- test/SemaObjCXX/blocks.mm
+++ test/SemaObjCXX/blocks.mm
@@ -169,3 +169,17 @@
return b; // expected-error {{no viable conversion from returned value of type 'MoveBlockVariable::B0' to function return type 'MoveBlockVariable::B1'}}
}
}
+
+namespace DefaultArg {
+void test() {
+ id x;
+ void func0(id a0, id a1 = ^{ (void) }); // expected-error {{default argument references parameter 'a0'}}
+ void func1(id a0, id a1 = ^{ (void) }); // expected-error {{default argument references local variable 'x' of enclosing function}}
+ void func2(id a0, id a1 = ^{ (void)sizeof(a0); });
+ void func3(id a0 = ^{ (void)sizeof(x); });
+ void func4(id a0, id a1 = ^{
+^{ (void) }(); // expected-error {{default argument references parameter 'a0'}}
+[=](){ (void) }(); // expected-error {{default argument references parameter 'a0'}}
+ });
+}
+}
Index: test/SemaCXX/default1.cpp
===
--- test/SemaCXX/default1.cpp
+++ test/SemaCXX/default1.cpp
@@ -78,3 +78,60 @@
void PR20769_b(int = 1);
void PR20769_b() { void PR20769_b(int = 2); }
+
+#if __cplusplus >= 201103L // C++11 or later
+struct S2 {
+ template
+ S2(T&&) {}
+};
+
+template
+void func0(int a0, S2 a1 = [](){ (void) }); // expected-error {{default argument references parameter 'a0'}}
+
+template
+void func1(T a0, int a1, S2 a2 = _Generic((a0), default: [](){ (void) }, int: 0)); // expected-error {{default argument references parameter 'a1'}}
+
+template
+void func2(S2 a0 = [](){
+ int t; [](){ (void)}();
+});
+
+template
+void func3(int a0, S2 a1 = [](){
+ [=](){ (void)}(); // expected-error {{default argument references parameter 'a0'}}
+});
+
+double d;
+
+void test1() {
+ int i;
+ float f;
+ void foo0(int a0 = _Generic((f), double: d, float: f)); // expected-error {{default argument references local variable 'f' of enclosing function}}
+ void foo1(int a0 = _Generic((d), double: d, float: f));
+ void foo2(int a0 = _Generic((i), int: d, float: f));
+ void foo3(int a0 = _Generic((i), default: d, float: f));
+
+ void foo4(S2 a0 = [&](){ (void) }); // expected-error {{lambda expression in default argument cannot capture any entity}}
+ void foo5(S2 a0 = [](){
+// No warning about capture list of a lambda expression defined in a block scope.
+int t; [](){ (void)}();
+ });
+ void foo6(int a0, S2 a1 = [](){
+// No warning about local variables or parameters referenced by an
+// unevaluated expression.
+int t = sizeof({i, a0;});
+ });
+ void foo6(S2 a0 = [](){
+int i; // expected-note {{'i' declared here}}
+void foo7(int a0, // expected-note {{'a0' declared here}}
+ S2 a1 = [](){ (void) }); // expected-error {{variable 'a0' cannot be implicitly captured in a lambda with no capture-default specified}} expected-error {{default argument references parameter 'a0'}} expected-note {{lambda
