[PATCH] D35941: Fix -Wshadow false positives with function-local classes.
alexfh added a comment.
In https://reviews.llvm.org/D35941#838633, @rsmith wrote:
> In https://reviews.llvm.org/D35941#823524, @alexfh wrote:
>
> > IIUC, most cases where -Wshadow warnings are issued is when a declaration
> > from an enclosing scope would be accessible if there was no declaration
> > that shadows it. In this case the the local variable of a function would
> > not be accessible inside the local class anyway
>
>
> That's not strictly true; the variable can be accessed in unevaluated
> operands, and in code doing so, a `-Wshadow` warning might (theoretically) be
> useful:
>
> void f(SomeComplexType val) {
> struct A {
> decltype(val)
> void g(int val) {
> decltype(val) *p =
> }
> } a = {val};
> }
>
Good to know!
> That said, suppressing the warning seems like a good thing in the common
> case. We've discussed the idea of deferring some `-Wshadow` warnings until we
> see a use; if someone cares about this case, we could consider warning only
> if the shadowed variable is actually used in an unevaluated operand.
Sounds reasonable.
Repository:
rL LLVM
https://reviews.llvm.org/D35941
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35941: Fix -Wshadow false positives with function-local classes.
rsmith added a comment.
In https://reviews.llvm.org/D35941#823524, @alexfh wrote:
> IIUC, most cases where -Wshadow warnings are issued is when a declaration
> from an enclosing scope would be accessible if there was no declaration that
> shadows it. In this case the the local variable of a function would not be
> accessible inside the local class anyway
That's not strictly true; the variable can be accessed in unevaluated operands,
and in code doing so, a `-Wshadow` warning might (theoretically) be useful:
void f(SomeComplexType val) {
struct A {
decltype(val)
void g(int val) {
decltype(val) *p =
}
} a = {val};
}
That said, suppressing the warning seems like a good thing in the common case.
We've discussed the idea of deferring some `-Wshadow` warnings until we see a
use; if someone cares about this case, we could consider warning only if the
shadowed variable is actually used in an unevaluated operand.
Repository:
rL LLVM
https://reviews.llvm.org/D35941
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35941: Fix -Wshadow false positives with function-local classes.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL309569: Fix -Wshadow false positives with function-local
classes. (authored by alexfh).
Repository:
rL LLVM
https://reviews.llvm.org/D35941
Files:
cfe/trunk/lib/Sema/SemaDecl.cpp
cfe/trunk/test/SemaCXX/warn-shadow.cpp
Index: cfe/trunk/lib/Sema/SemaDecl.cpp
===
--- cfe/trunk/lib/Sema/SemaDecl.cpp
+++ cfe/trunk/lib/Sema/SemaDecl.cpp
@@ -6999,6 +6999,21 @@
return;
}
}
+
+ if (cast(ShadowedDecl)->hasLocalStorage()) {
+// A variable can't shadow a local variable in an enclosing scope, if
+// they are separated by a non-capturing declaration context.
+for (DeclContext *ParentDC = NewDC;
+ ParentDC && !ParentDC->Equals(OldDC);
+ ParentDC = getLambdaAwareParentOfDeclContext(ParentDC)) {
+ // Only block literals, captured statements, and lambda expressions
+ // can capture; other scopes don't.
+ if (!isa(ParentDC) && !isa(ParentDC) &&
+ !isLambdaCallOperator(ParentDC)) {
+return;
+ }
+}
+ }
}
}
Index: cfe/trunk/test/SemaCXX/warn-shadow.cpp
===
--- cfe/trunk/test/SemaCXX/warn-shadow.cpp
+++ cfe/trunk/test/SemaCXX/warn-shadow.cpp
@@ -213,3 +213,12 @@
void handleLinkageSpec() {
typedef void externC; // expected-warning {{declaration shadows a typedef in
the global namespace}}
}
+
+namespace PR33947 {
+void f(int a) {
+ struct A {
+void g(int a) {}
+A() { int a; }
+ };
+}
+}
Index: cfe/trunk/lib/Sema/SemaDecl.cpp
===
--- cfe/trunk/lib/Sema/SemaDecl.cpp
+++ cfe/trunk/lib/Sema/SemaDecl.cpp
@@ -6999,6 +6999,21 @@
return;
}
}
+
+ if (cast(ShadowedDecl)->hasLocalStorage()) {
+// A variable can't shadow a local variable in an enclosing scope, if
+// they are separated by a non-capturing declaration context.
+for (DeclContext *ParentDC = NewDC;
+ ParentDC && !ParentDC->Equals(OldDC);
+ ParentDC = getLambdaAwareParentOfDeclContext(ParentDC)) {
+ // Only block literals, captured statements, and lambda expressions
+ // can capture; other scopes don't.
+ if (!isa(ParentDC) && !isa(ParentDC) &&
+ !isLambdaCallOperator(ParentDC)) {
+return;
+ }
+}
+ }
}
}
Index: cfe/trunk/test/SemaCXX/warn-shadow.cpp
===
--- cfe/trunk/test/SemaCXX/warn-shadow.cpp
+++ cfe/trunk/test/SemaCXX/warn-shadow.cpp
@@ -213,3 +213,12 @@
void handleLinkageSpec() {
typedef void externC; // expected-warning {{declaration shadows a typedef in the global namespace}}
}
+
+namespace PR33947 {
+void f(int a) {
+ struct A {
+void g(int a) {}
+A() { int a; }
+ };
+}
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35941: Fix -Wshadow false positives with function-local classes.
Quuxplusone accepted this revision. Quuxplusone added a comment. > But if I'm overseeing reasons to issue a warning in this case, we could add > an analogue of `-Wshadow-uncaptured-local` for this case. WDYT? I still think "any warning" is a marginally better UX than "no warning" on the particular code in question; but of course I defer to Reid/Richard/GCC on the practicalities. Adding a new warning option might not be worth the trouble. :) https://reviews.llvm.org/D35941 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35941: Fix -Wshadow false positives with function-local classes.
rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. > Another data point is that GCC doesn't warn in this case. That seems like a reasonable tie breaker when implementing these kinds of style-enforcement warnings. :) https://reviews.llvm.org/D35941 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35941: Fix -Wshadow false positives with function-local classes.
alexfh added a comment. In https://reviews.llvm.org/D35941#823020, @rnk wrote: > I'm not really sure this is a bug. The point of -Wshadow is to warn on valid > but possibly confusing code. Shadowing a variable is perfectly legal, but > people think it's confusing, so we implemented this warning. Are we concerned > that people might get confused between the different local variables here? IIUC, most cases where -Wshadow warnings are issued is when a declaration from an enclosing scope would be accessible if there was no declaration that shadows it. In this case the the local variable of a function would not be accessible inside the local class anyway, so the inner variable with the same name is not confusing (at least, much less confusing than if it would prevent access to the variable of an outer scope). This is close to shadowing of uncaptured locals in lambdas, but it seems to have even less potential for being misleading. We had a few of requests internally to mute `-Wshadow` for this case. Another data point is that GCC doesn't warn in this case. But if I'm overseeing reasons to issue a warning in this case, we could add an analogue of `-Wshadow-uncaptured-local` for this case. WDYT? https://reviews.llvm.org/D35941 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35941: Fix -Wshadow false positives with function-local classes.
rnk added a comment. I'm not really sure this is a bug. The point of -Wshadow is to warn on valid but possibly confusing code. Shadowing a variable is perfectly legal, but people think it's confusing, so we implemented this warning. Are we concerned that people might get confused between the different local variables here? https://reviews.llvm.org/D35941 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35941: Fix -Wshadow false positives with function-local classes.
alexfh created this revision.
Fixes http://llvm.org/PR33947.
https://godbolt.org/g/54XRMT
void f(int a) {
struct A {
void g(int a) {}
A() { int a; }
};
}
3 : :3:16: warning: declaration shadows a local variable [-Wshadow]
void g(int a) {}
^
1 : :1:12: note: previous declaration is here
void f(int a) {
^
4 : :4:15: warning: declaration shadows a local variable [-Wshadow]
A() { int a; }
^
1 : :1:12: note: previous declaration is here
void f(int a) {
^
2 warnings generated.
The local variable `a` of the function `f` can't be accessed from a method of
the function-local class A, thus no shadowing occurs and no diagnostic is
needed.
https://reviews.llvm.org/D35941
Files:
lib/Sema/SemaDecl.cpp
test/SemaCXX/warn-shadow.cpp
Index: test/SemaCXX/warn-shadow.cpp
===
--- test/SemaCXX/warn-shadow.cpp
+++ test/SemaCXX/warn-shadow.cpp
@@ -213,3 +213,12 @@
void handleLinkageSpec() {
typedef void externC; // expected-warning {{declaration shadows a typedef in
the global namespace}}
}
+
+namespace PR33947 {
+void f(int a) {
+ struct A {
+void g(int a) {}
+A() { int a; }
+ };
+}
+}
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -6999,6 +6999,21 @@
return;
}
}
+
+ if (cast(ShadowedDecl)->hasLocalStorage()) {
+// A variable can't shadow a local variable in an enclosing scope, if
+// they are separated by a non-capturing declaration context.
+for (DeclContext *ParentDC = NewDC;
+ ParentDC && !ParentDC->Equals(OldDC);
+ ParentDC = getLambdaAwareParentOfDeclContext(ParentDC)) {
+ // Only block literals, captured statements, and lambda expressions
+ // can capture; other scopes don't.
+ if (!isa(ParentDC) && !isa(ParentDC) &&
+ !isLambdaCallOperator(ParentDC)) {
+return;
+ }
+}
+ }
}
}
Index: test/SemaCXX/warn-shadow.cpp
===
--- test/SemaCXX/warn-shadow.cpp
+++ test/SemaCXX/warn-shadow.cpp
@@ -213,3 +213,12 @@
void handleLinkageSpec() {
typedef void externC; // expected-warning {{declaration shadows a typedef in the global namespace}}
}
+
+namespace PR33947 {
+void f(int a) {
+ struct A {
+void g(int a) {}
+A() { int a; }
+ };
+}
+}
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -6999,6 +6999,21 @@
return;
}
}
+
+ if (cast(ShadowedDecl)->hasLocalStorage()) {
+// A variable can't shadow a local variable in an enclosing scope, if
+// they are separated by a non-capturing declaration context.
+for (DeclContext *ParentDC = NewDC;
+ ParentDC && !ParentDC->Equals(OldDC);
+ ParentDC = getLambdaAwareParentOfDeclContext(ParentDC)) {
+ // Only block literals, captured statements, and lambda expressions
+ // can capture; other scopes don't.
+ if (!isa(ParentDC) && !isa(ParentDC) &&
+ !isLambdaCallOperator(ParentDC)) {
+return;
+ }
+}
+ }
}
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
