[PATCH] D52400: Improve -Wshadow warnings with enumerators
sberg added a comment. > I've silenced this scenario in r344898, thank you for raising the issue! thanks! works fine for me https://reviews.llvm.org/D52400 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52400: Improve -Wshadow warnings with enumerators
aaron.ballman added a comment. In https://reviews.llvm.org/D52400#1267731, @aaron.ballman wrote: > In https://reviews.llvm.org/D52400#1267499, @sberg wrote: > > > (and in any case, "declaration shadows a variable" sounds wrong when the > > shadowed entity is a class type. thats why I thought something is not quite > > right with this new code yet) > > > Definitely agreed. I think I'm convinced that this should be silenced when > the conflict is with a type rather than a value. I'll try to look into this > next week when I'm back from WG14 meetings. I've silenced this scenario in r344898, thank you for raising the issue! https://reviews.llvm.org/D52400 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52400: Improve -Wshadow warnings with enumerators
aaron.ballman added a comment.
In https://reviews.llvm.org/D52400#1267499, @sberg wrote:
> In https://reviews.llvm.org/D52400#1266341, @aaron.ballman wrote:
>
> > In https://reviews.llvm.org/D52400#1266307, @sberg wrote:
> >
> > >
> >
>
>
> [...]
>
> > Then again, this is a case where you don't get any error but you do get a
> > silent behavioral ambiguity without the current enumerator shadow
> > diagnostic:
> >
> > struct S1;
> > struct S2;
> > struct S3 {
> > void S1();
> > enum { S2 };
> >
> > void f(decltype(S2) s);
> > };
> >
> >
> > So there are cases where this behavior can be somewhat useful.
>
> but decltype(S2) is a syntax error when S2 names a struct type, no?
Ugh, you're absolutely right.
>>> (ran into such a new -Wshadow while compiling LibreOffice)
>>
>> Was it a frequent/annoying occurrence?
>
> there was less than 20 cases overall. about half were "good" warnings about
> clashing enumerators from different non-scoped enums. the others were
> "unhelpful" ones about clashes with class names, two of them in stable
> interface code that cant be changed (so would need ugly #pragma clang warning
> decorations), one of them even about entities in unrelated include files, so
> whether a warning is emitted depends on the order in which the files happen
> to get included in a TU
Thanks, that's good information!
> (and in any case, "declaration shadows a variable" sounds wrong when the
> shadowed entity is a class type. thats why I thought something is not quite
> right with this new code yet)
Definitely agreed. I think I'm convinced that this should be silenced when the
conflict is with a type rather than a value. I'll try to look into this next
week when I'm back from WG14 meetings.
https://reviews.llvm.org/D52400
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52400: Improve -Wshadow warnings with enumerators
sberg added a comment.
In https://reviews.llvm.org/D52400#1266341, @aaron.ballman wrote:
> In https://reviews.llvm.org/D52400#1266307, @sberg wrote:
>
> >
>
[...]
> Then again, this is a case where you don't get any error but you do get a
> silent behavioral ambiguity without the current enumerator shadow diagnostic:
>
> struct S1;
> struct S2;
> struct S3 {
> void S1();
> enum { S2 };
>
> void f(decltype(S2) s);
> };
>
>
> So there are cases where this behavior can be somewhat useful.
but decltype(S2) is a syntax error when S2 names a struct type, no?
>> (ran into such a new -Wshadow while compiling LibreOffice)
>
> Was it a frequent/annoying occurrence?
there was less than 20 cases overall. about half were "good" warnings about
clashing enumerators from different non-scoped enums. the others were
"unhelpful" ones about clashes with class names, two of them in stable
interface code that cant be changed (so would need ugly #pragma clang warning
decorations), one of them even about entities in unrelated include files, so
whether a warning is emitted depends on the order in which the files happen to
get included in a TU
(and in any case, "declaration shadows a variable" sounds wrong when the
shadowed entity is a class type. thats why I thought something is not quite
right with this new code yet)
https://reviews.llvm.org/D52400
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52400: Improve -Wshadow warnings with enumerators
aaron.ballman added a comment.
In https://reviews.llvm.org/D52400#1266307, @sberg wrote:
> doesnt this make -Wshadow more aggressive for enumerators than for other
> entities?
It does, but whether that's an issue with the enumerator shadow diagnosing or
with the other entities not diagnosing, I'm less clear. Consider this slight
modification to your code:
struct S1;
struct S2;
struct S3 {
void S1();
enum { S2 };
void f(S2 s);
};
On the one hand, the warning is telling you about a problem before you hit it.
However, this code will err on the declaration of `S::f()` anyway because `S2`
is not a type, so the warning doesn't get us all *that* much benefit.
Then again, this is a case where you don't get any error but you do get a
silent behavioral ambiguity without the current enumerator shadow diagnostic:
struct S1;
struct S2;
struct S3 {
void S1();
enum { S2 };
void f(decltype(S2) s);
};
So there are cases where this behavior can be somewhat useful.
> (ran into such a new -Wshadow while compiling LibreOffice)
Was it a frequent/annoying occurrence?
https://reviews.llvm.org/D52400
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52400: Improve -Wshadow warnings with enumerators
sberg added a comment.
doesnt this make -Wshadow more aggressive for enumerators than for other
entities?
~ cat test17.cc
struct S1;
struct S2;
struct S3 {
void S1();
enum { S2 };
};
~ llvm/inst/bin/clang++ -fsyntax-only -Wshadow test17.cc
test17.cc:5:10: warning: declaration shadows a variable in the global
namespace
[-Wshadow]
enum { S2 };
^
test17.cc:2:8: note: previous declaration is here
struct S2;
^
1 warning generated.
warns about enumerator S2 but not about similar function S1
(ran into such a new -Wshadow while compiling LibreOffice)
https://reviews.llvm.org/D52400
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52400: Improve -Wshadow warnings with enumerators
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. Committed in r344259 on @erik.pilkington 's LGTM. Self-accepting so I can close the review. https://reviews.llvm.org/D52400 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52400: Improve -Wshadow warnings with enumerators
aaron.ballman added inline comments.
Comment at: lib/Sema/SemaDecl.cpp:16320
+// Check for other kinds of shadowing not already handled.
+CheckShadow(New, PrevDecl, R);
+
erik.pilkington wrote:
> I don't think we should do this for scoped enums in C++, i.e. this should be
> fine:
> ```
> int Foo;
> enum class X { Foo };
> ```
>
> Can you enclose this in `if (!TheEnumDecl->isScoped())` and add a test? Other
> than that, LGTM!
Thanks, that's a great point! I've corrected in an updated patch.
https://reviews.llvm.org/D52400
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52400: Improve -Wshadow warnings with enumerators
aaron.ballman updated this revision to Diff 167507.
aaron.ballman marked an inline comment as done.
aaron.ballman added a comment.
Updating based on review feedback.
https://reviews.llvm.org/D52400
Files:
lib/Sema/SemaDecl.cpp
test/Sema/warn-shadow.c
test/SemaCXX/warn-shadow.cpp
Index: test/SemaCXX/warn-shadow.cpp
===
--- test/SemaCXX/warn-shadow.cpp
+++ test/SemaCXX/warn-shadow.cpp
@@ -222,3 +222,6 @@
};
}
}
+
+int PR24718;
+enum class X { PR24718 }; // Ok, not shadowing
Index: test/Sema/warn-shadow.c
===
--- test/Sema/warn-shadow.c
+++ test/Sema/warn-shadow.c
@@ -59,3 +59,8 @@
void test8() {
int bob; // expected-warning {{declaration shadows a variable in the global
scope}}
}
+
+enum PR24718_1{pr24718}; // expected-note {{previous declaration is here}}
+void PR24718(void) {
+ enum PR24718_2{pr24718}; // expected-warning {{declaration shadows a
variable in the global scope}}
+}
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -16290,8 +16290,10 @@
// Verify that there isn't already something declared with this name in this
// scope.
- NamedDecl *PrevDecl = LookupSingleName(S, Id, IdLoc, LookupOrdinaryName,
- ForVisibleRedeclaration);
+ LookupResult R(*this, Id, IdLoc, LookupOrdinaryName,
ForVisibleRedeclaration);
+ LookupName(R, S);
+ NamedDecl *PrevDecl = R.getAsSingle();
+
if (PrevDecl && PrevDecl->isTemplateParameter()) {
// Maybe we will complain about the shadowed template parameter.
DiagnoseTemplateParameterShadow(IdLoc, PrevDecl);
@@ -16314,6 +16316,11 @@
return nullptr;
if (PrevDecl) {
+if (!TheEnumDecl->isScoped()) {
+ // Check for other kinds of shadowing not already handled.
+ CheckShadow(New, PrevDecl, R);
+}
+
// When in C++, we may get a TagDecl with the same name; in this case the
// enum constant will 'hide' the tag.
assert((getLangOpts().CPlusPlus || !isa(PrevDecl)) &&
Index: test/SemaCXX/warn-shadow.cpp
===
--- test/SemaCXX/warn-shadow.cpp
+++ test/SemaCXX/warn-shadow.cpp
@@ -222,3 +222,6 @@
};
}
}
+
+int PR24718;
+enum class X { PR24718 }; // Ok, not shadowing
Index: test/Sema/warn-shadow.c
===
--- test/Sema/warn-shadow.c
+++ test/Sema/warn-shadow.c
@@ -59,3 +59,8 @@
void test8() {
int bob; // expected-warning {{declaration shadows a variable in the global scope}}
}
+
+enum PR24718_1{pr24718}; // expected-note {{previous declaration is here}}
+void PR24718(void) {
+ enum PR24718_2{pr24718}; // expected-warning {{declaration shadows a variable in the global scope}}
+}
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -16290,8 +16290,10 @@
// Verify that there isn't already something declared with this name in this
// scope.
- NamedDecl *PrevDecl = LookupSingleName(S, Id, IdLoc, LookupOrdinaryName,
- ForVisibleRedeclaration);
+ LookupResult R(*this, Id, IdLoc, LookupOrdinaryName, ForVisibleRedeclaration);
+ LookupName(R, S);
+ NamedDecl *PrevDecl = R.getAsSingle();
+
if (PrevDecl && PrevDecl->isTemplateParameter()) {
// Maybe we will complain about the shadowed template parameter.
DiagnoseTemplateParameterShadow(IdLoc, PrevDecl);
@@ -16314,6 +16316,11 @@
return nullptr;
if (PrevDecl) {
+if (!TheEnumDecl->isScoped()) {
+ // Check for other kinds of shadowing not already handled.
+ CheckShadow(New, PrevDecl, R);
+}
+
// When in C++, we may get a TagDecl with the same name; in this case the
// enum constant will 'hide' the tag.
assert((getLangOpts().CPlusPlus || !isa(PrevDecl)) &&
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52400: Improve -Wshadow warnings with enumerators
erik.pilkington added inline comments.
Comment at: lib/Sema/SemaDecl.cpp:16320
+// Check for other kinds of shadowing not already handled.
+CheckShadow(New, PrevDecl, R);
+
I don't think we should do this for scoped enums in C++, i.e. this should be
fine:
```
int Foo;
enum class X { Foo };
```
Can you enclose this in `if (!TheEnumDecl->isScoped())` and add a test? Other
than that, LGTM!
https://reviews.llvm.org/D52400
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52400: Improve -Wshadow warnings with enumerators
aaron.ballman added a reviewer: echristo. aaron.ballman added a comment. Ping? https://reviews.llvm.org/D52400 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52400: Improve -Wshadow warnings with enumerators
aaron.ballman created this revision.
aaron.ballman added reviewers: rsmith, dblaikie.
Currently, we do not check for enumerators that shadow other enumerators as
part of -Wshadow, but gcc does provide such a diagnostic for this case. This
is intended to catch shadowing issues like:
enum E1{e1};
void f(void) {
enum E2{e1};
}
This patch addresses PR24718.
https://reviews.llvm.org/D52400
Files:
lib/Sema/SemaDecl.cpp
test/Sema/warn-shadow.c
Index: test/Sema/warn-shadow.c
===
--- test/Sema/warn-shadow.c
+++ test/Sema/warn-shadow.c
@@ -59,3 +59,8 @@
void test8() {
int bob; // expected-warning {{declaration shadows a variable in the global
scope}}
}
+
+enum PR24718_1{pr24718}; // expected-note {{previous declaration is here}}
+void PR24718(void) {
+ enum PR24718_2{pr24718}; // expected-warning {{declaration shadows a
variable in the global scope}}
+}
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -16290,8 +16290,10 @@
// Verify that there isn't already something declared with this name in this
// scope.
- NamedDecl *PrevDecl = LookupSingleName(S, Id, IdLoc, LookupOrdinaryName,
- ForVisibleRedeclaration);
+ LookupResult R(*this, Id, IdLoc, LookupOrdinaryName,
ForVisibleRedeclaration);
+ LookupName(R, S);
+ NamedDecl *PrevDecl = R.getAsSingle();
+
if (PrevDecl && PrevDecl->isTemplateParameter()) {
// Maybe we will complain about the shadowed template parameter.
DiagnoseTemplateParameterShadow(IdLoc, PrevDecl);
@@ -16314,6 +16316,9 @@
return nullptr;
if (PrevDecl) {
+// Check for other kinds of shadowing not already handled.
+CheckShadow(New, PrevDecl, R);
+
// When in C++, we may get a TagDecl with the same name; in this case the
// enum constant will 'hide' the tag.
assert((getLangOpts().CPlusPlus || !isa(PrevDecl)) &&
Index: test/Sema/warn-shadow.c
===
--- test/Sema/warn-shadow.c
+++ test/Sema/warn-shadow.c
@@ -59,3 +59,8 @@
void test8() {
int bob; // expected-warning {{declaration shadows a variable in the global scope}}
}
+
+enum PR24718_1{pr24718}; // expected-note {{previous declaration is here}}
+void PR24718(void) {
+ enum PR24718_2{pr24718}; // expected-warning {{declaration shadows a variable in the global scope}}
+}
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -16290,8 +16290,10 @@
// Verify that there isn't already something declared with this name in this
// scope.
- NamedDecl *PrevDecl = LookupSingleName(S, Id, IdLoc, LookupOrdinaryName,
- ForVisibleRedeclaration);
+ LookupResult R(*this, Id, IdLoc, LookupOrdinaryName, ForVisibleRedeclaration);
+ LookupName(R, S);
+ NamedDecl *PrevDecl = R.getAsSingle();
+
if (PrevDecl && PrevDecl->isTemplateParameter()) {
// Maybe we will complain about the shadowed template parameter.
DiagnoseTemplateParameterShadow(IdLoc, PrevDecl);
@@ -16314,6 +16316,9 @@
return nullptr;
if (PrevDecl) {
+// Check for other kinds of shadowing not already handled.
+CheckShadow(New, PrevDecl, R);
+
// When in C++, we may get a TagDecl with the same name; in this case the
// enum constant will 'hide' the tag.
assert((getLangOpts().CPlusPlus || !isa(PrevDecl)) &&
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
