| Message ID | 1473598955-9246-1-git-send-email-mjw@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 11/09/16 14:02, Mark Wielaard wrote: > -Wshadow-local which warns if a local variable shadows another local > variable or parameter, > > -Wshadow-compatible-local which warns if a local variable shadows > another local variable or parameter whose type is compatible with that > of the shadowing variable. I honestly don't see the need for the second flag. Why not make Wshadow, or at least Wshadow-local, work in this way by default? Warning for shadowed variables that will nevertheless trigger errors/warnings if used wrongly seems not very useful anyway. > + /* If '-Wshadow-compatible-local' is specified without other > + -Wshadow flags, we will warn only when the types of the > + shadowing variable (i.e. new_decl) and the shadowed variable > + (old_decl) are compatible. */ > + if (comptypes (TREE_TYPE (old_decl), TREE_TYPE (new_decl))) > + warning_code = OPT_Wshadow_compatible_local; > + else > + warning_code = OPT_Wshadow_local; > + warned = warning (warning_code, > + "declaration of %q+D shadows a parameter", > + new_decl); Please don't use +D. Use warning_at with DECL_SOURCE_LOCATION(new_decl). See: https://gcc.gnu.org/wiki/DiagnosticsGuidelines#Locations > + /* If '-Wshadow-compatible-local' is specified without other > + -Wshadow flags, we will warn only when the type of the > + shadowing variable (i.e. x) can be converted to that of > + the shadowed parameter (oldlocal). The reason why we only > + check if x's type can be converted to oldlocal's type > + (but not the other way around) is because when users > + accidentally shadow a parameter, more than often they > + would use the variable thinking (mistakenly) it's still > + the parameter. It would be rare that users would use the > + variable in the place that expects the parameter but > + thinking it's a new decl. */ As said above, IMHO, this behavior should be the default of -Wshadow, or at least, of -Wshadow-local. The current behavior only leads to people not using -Wshadow (and us not including it in -Wall -Wextra). There is a Linus rant from some years ago that explains vehemently why Wshadow is useless in its current form. > +@item -Wshadow-compatible-local > +@opindex Wshadow-compatible-local > +@opindex Wno-shadow-compatible-local > +Warn when a local variable shadows another local variable or parameter > +whose type is compatible with that of the shadowing variable. In C++, > +type compatibility here means the type of the shadowing variable can be > +converted to that of the shadowed variable. The creation of this flag > +(in addition to @option{-Wshadow-local}) is based on the idea that when > +a local variable shadows another one of incompatible type, it is most > +likely intentional, not a bug or typo, as shown in the following example: -Wshadow-compatible-local seems safe enough to be enabled by -Wall (or -Wextra). Options not enabled by either are rarely used (and, hence, rarely tested). Cheers, Manuel.
On 9/11/16, Manuel López-Ibáñez <lopezibanez@gmail.com> wrote: > On 11/09/16 14:02, Mark Wielaard wrote: >> -Wshadow-local which warns if a local variable shadows another local >> variable or parameter, >> >> -Wshadow-compatible-local which warns if a local variable shadows >> another local variable or parameter whose type is compatible with that >> of the shadowing variable. > > I honestly don't see the need for the second flag. Why not make Wshadow, or at > least Wshadow-local, work in this way by default? Warning for shadowed > variables that will nevertheless trigger errors/warnings if used wrongly > seems not very useful anyway. > > It helps for readability and helps pre-emptively catch those other errors/warnings that come from using them wrongly before they occur in a more confusing manner. Plus more granularity makes it easier to manage the workload of warning-silencing. >> + /* If '-Wshadow-compatible-local' is specified without other >> + -Wshadow flags, we will warn only when the types of the >> + shadowing variable (i.e. new_decl) and the shadowed variable >> + (old_decl) are compatible. */ >> + if (comptypes (TREE_TYPE (old_decl), TREE_TYPE (new_decl))) >> + warning_code = OPT_Wshadow_compatible_local; >> + else >> + warning_code = OPT_Wshadow_local; >> + warned = warning (warning_code, >> + "declaration of %q+D shadows a parameter", >> + new_decl); > > Please don't use +D. Use warning_at with DECL_SOURCE_LOCATION(new_decl). > See: https://gcc.gnu.org/wiki/DiagnosticsGuidelines#Locations > > >> + /* If '-Wshadow-compatible-local' is specified without other >> + -Wshadow flags, we will warn only when the type of the >> + shadowing variable (i.e. x) can be converted to that of >> + the shadowed parameter (oldlocal). The reason why we only >> + check if x's type can be converted to oldlocal's type >> + (but not the other way around) is because when users >> + accidentally shadow a parameter, more than often they >> + would use the variable thinking (mistakenly) it's still >> + the parameter. It would be rare that users would use the >> + variable in the place that expects the parameter but >> + thinking it's a new decl. */ > > As said above, IMHO, this behavior should be the default of -Wshadow, or at > least, of -Wshadow-local. The current behavior only leads to people not > using -Wshadow (and us not including it in -Wall -Wextra). There is a Linus rant > from some years ago that explains vehemently why Wshadow is useless in its > current form. > >> +@item -Wshadow-compatible-local >> +@opindex Wshadow-compatible-local >> +@opindex Wno-shadow-compatible-local >> +Warn when a local variable shadows another local variable or parameter >> +whose type is compatible with that of the shadowing variable. In C++, >> +type compatibility here means the type of the shadowing variable can be >> +converted to that of the shadowed variable. The creation of this flag >> +(in addition to @option{-Wshadow-local}) is based on the idea that when >> +a local variable shadows another one of incompatible type, it is most >> +likely intentional, not a bug or typo, as shown in the following >> example: > > -Wshadow-compatible-local seems safe enough to be enabled by -Wall (or > -Wextra). Options not enabled by either are rarely used (and, hence, rarely > tested). > > Cheers, > > Manuel. > I see lots of GNU projects at least using the gnulib manywarnings.m4 autoconf macros in their configure script, and that enables a lot more warnings than just -Wall and -Wextra. So I think the test coverage for warnings outside of -Wall or -Wextra is better than you might think.
On Mon, Sep 12, 2016 at 7:05 AM, Eric Gallager <egall@gwmail.gwu.edu> wrote: > On 9/11/16, Manuel López-Ibáñez <lopezibanez@gmail.com> wrote: >> On 11/09/16 14:02, Mark Wielaard wrote: >>> -Wshadow-local which warns if a local variable shadows another local >>> variable or parameter, >>> >>> -Wshadow-compatible-local which warns if a local variable shadows >>> another local variable or parameter whose type is compatible with that >>> of the shadowing variable. Hi Mark, Thank you for doing this! >> I honestly don't see the need for the second flag. Why not make Wshadow, or at >> least Wshadow-local, work in this way by default? Warning for shadowed >> variables that will nevertheless trigger errors/warnings if used wrongly >> seems not very useful anyway. > > It helps for readability and helps pre-emptively catch those other > errors/warnings that come from using them wrongly before they occur in > a more confusing manner. Plus more granularity makes it easier to > manage the workload of warning-silencing. The new warnings will be especially welcome in C++ code. -Wshadow is fine for most C code, but in C++ it is very common to have method names that shadow class data member names and/or local variables in any member function definition. Thus, using -Wshadow in C++ code often forces undesirable (and unscalable) renaming to avoid such shadowing, while the only important type of shadowing is that caught by the new options. Many of us want the benefit of the new options (spotting bad, easily-fixed code), without having to incur the renaming (often hurting readability/maintainability) required to enable -Werror=shadow.
On Mon, Sep 12, 2016 at 1:38 PM, Jim Meyering <jim@meyering.net> wrote: > On Mon, Sep 12, 2016 at 7:05 AM, Eric Gallager <egall@gwmail.gwu.edu> wrote: >> On 9/11/16, Manuel López-Ibáñez <lopezibanez@gmail.com> wrote: >>> On 11/09/16 14:02, Mark Wielaard wrote: >>>> -Wshadow-local which warns if a local variable shadows another local >>>> variable or parameter, >>>> >>>> -Wshadow-compatible-local which warns if a local variable shadows >>>> another local variable or parameter whose type is compatible with that >>>> of the shadowing variable. > > Hi Mark, > Thank you for doing this! > >>> I honestly don't see the need for the second flag. Why not make Wshadow, or at >>> least Wshadow-local, work in this way by default? Warning for shadowed >>> variables that will nevertheless trigger errors/warnings if used wrongly >>> seems not very useful anyway. >> >> It helps for readability and helps pre-emptively catch those other >> errors/warnings that come from using them wrongly before they occur in >> a more confusing manner. Plus more granularity makes it easier to >> manage the workload of warning-silencing. > > The new warnings will be especially welcome in C++ code. > -Wshadow is fine for most C code, but in C++ it is very common to have > method names that shadow class data member names and/or local > variables in any member function definition. Thus, using -Wshadow in > C++ code often forces undesirable (and unscalable) renaming to avoid > such shadowing, while the only important type of shadowing is that > caught by the new options. Many of us want the benefit of the new > options (spotting bad, easily-fixed code), without having to incur the > renaming (often hurting readability/maintainability) required to > enable -Werror=shadow. I wonder about spelling the options as -Wshadow={local,compatible-local} rather than with a dash, but otherwise the patch looks fine. Jason
On Wed, 2016-09-14 at 00:00 -0400, Jason Merrill wrote: > I wonder about spelling the options as > -Wshadow={local,compatible-local} rather than with a dash, but > otherwise the patch looks fine. That is a much nicer way to write the option. But if I do that I would like to keep the old names as aliases because Google already ships a gcc that accepts -Wshadow-local and -Wshadow-compatible-local and you can find programs that already probe for those names in their configure scripts. Can I make the existing names hidden aliases by marking them Undocumented in the .opt file? Or is that too contrived/ugly? Thanks, Mark
On 9/14/16, Jason Merrill <jason@redhat.com> wrote: > On Mon, Sep 12, 2016 at 1:38 PM, Jim Meyering <jim@meyering.net> wrote: >> On Mon, Sep 12, 2016 at 7:05 AM, Eric Gallager <egall@gwmail.gwu.edu> >> wrote: >>> On 9/11/16, Manuel López-Ibáñez <lopezibanez@gmail.com> wrote: >>>> On 11/09/16 14:02, Mark Wielaard wrote: >>>>> -Wshadow-local which warns if a local variable shadows another local >>>>> variable or parameter, >>>>> >>>>> -Wshadow-compatible-local which warns if a local variable shadows >>>>> another local variable or parameter whose type is compatible with >>>>> that >>>>> of the shadowing variable. >> >> Hi Mark, >> Thank you for doing this! >> >>>> I honestly don't see the need for the second flag. Why not make Wshadow, >>>> or at >>>> least Wshadow-local, work in this way by default? Warning for shadowed >>>> variables that will nevertheless trigger errors/warnings if used >>>> wrongly >>>> seems not very useful anyway. >>> >>> It helps for readability and helps pre-emptively catch those other >>> errors/warnings that come from using them wrongly before they occur in >>> a more confusing manner. Plus more granularity makes it easier to >>> manage the workload of warning-silencing. >> >> The new warnings will be especially welcome in C++ code. >> -Wshadow is fine for most C code, but in C++ it is very common to have >> method names that shadow class data member names and/or local >> variables in any member function definition. Thus, using -Wshadow in >> C++ code often forces undesirable (and unscalable) renaming to avoid >> such shadowing, while the only important type of shadowing is that >> caught by the new options. Many of us want the benefit of the new >> options (spotting bad, easily-fixed code), without having to incur the >> renaming (often hurting readability/maintainability) required to >> enable -Werror=shadow. > > I wonder about spelling the options as > -Wshadow={local,compatible-local} rather than with a dash, but > otherwise the patch looks fine. > > Jason > What would the current behavior be called? Maybe -Wshadow={all,local,compatible-local} instead? Also remember -Werror: with another '=' you could end up with 2 of them with something like -Werror=shadow=local which looks more confusing than the hyphenated version. Eric
On Wed, Sep 14, 2016 at 1:30 AM, Mark Wielaard <mjw@redhat.com> wrote: > On Wed, 2016-09-14 at 00:00 -0400, Jason Merrill wrote: >> I wonder about spelling the options as >> -Wshadow={local,compatible-local} rather than with a dash, but >> otherwise the patch looks fine. > > That is a much nicer way to write the option. But if I do that I would > like to keep the old names as aliases because Google already ships a gcc > that accepts -Wshadow-local and -Wshadow-compatible-local and you can > find programs that already probe for those names in their configure > scripts. Can I make the existing names hidden aliases by marking them > Undocumented in the .opt file? Or is that too contrived/ugly? I don't have any opinion as to what the option names should be, but I don't see the fact that Google's GCC has different option names as a concern. That GCC is only used within Google, and Google is moving to LLVM in any case. Changing the option names in GCC trunk is not a problem for anybody. Ian
On Wed, 2016-09-14 at 05:39 -0700, Ian Lance Taylor wrote: > On Wed, Sep 14, 2016 at 1:30 AM, Mark Wielaard <mjw@redhat.com> wrote: > > On Wed, 2016-09-14 at 00:00 -0400, Jason Merrill wrote: > >> I wonder about spelling the options as > >> -Wshadow={local,compatible-local} rather than with a dash, but > >> otherwise the patch looks fine. > > > > That is a much nicer way to write the option. But if I do that I would > > like to keep the old names as aliases because Google already ships a gcc > > that accepts -Wshadow-local and -Wshadow-compatible-local and you can > > find programs that already probe for those names in their configure > > scripts. Can I make the existing names hidden aliases by marking them > > Undocumented in the .opt file? Or is that too contrived/ugly? > > I don't have any opinion as to what the option names should be, but I > don't see the fact that Google's GCC has different option names as a > concern. That GCC is only used within Google Google did release a gcc with those warning options (I believe as part of the NDK) and there are various projects out there (firefox seems one of them) that check to see if these options are available before enabling/disabling them (I don't know if other compilers implemented them). Given that there are public sources out there that already seem to use/test for these option names I would prefer to keep the compatibility. Cheers, Mark
On Wed, Sep 14, 2016 at 5:49 AM, Mark Wielaard <mjw@redhat.com> wrote: > On Wed, 2016-09-14 at 05:39 -0700, Ian Lance Taylor wrote: >> On Wed, Sep 14, 2016 at 1:30 AM, Mark Wielaard <mjw@redhat.com> wrote: >> > On Wed, 2016-09-14 at 00:00 -0400, Jason Merrill wrote: >> >> I wonder about spelling the options as >> >> -Wshadow={local,compatible-local} rather than with a dash, but >> >> otherwise the patch looks fine. >> > >> > That is a much nicer way to write the option. But if I do that I would >> > like to keep the old names as aliases because Google already ships a gcc >> > that accepts -Wshadow-local and -Wshadow-compatible-local and you can >> > find programs that already probe for those names in their configure >> > scripts. Can I make the existing names hidden aliases by marking them >> > Undocumented in the .opt file? Or is that too contrived/ugly? >> >> I don't have any opinion as to what the option names should be, but I >> don't see the fact that Google's GCC has different option names as a >> concern. That GCC is only used within Google > > Google did release a gcc with those warning options (I believe as part > of the NDK) and there are various projects out there (firefox seems one > of them) that check to see if these options are available before > enabling/disabling them (I don't know if other compilers implemented > them). Given that there are public sources out there that already seem > to use/test for these option names I would prefer to keep the > compatibility. Thanks again for working on this. I have been using these new options (locally patched) to good effect. While the vast majority of warning-triggering code has been technically correct, using these has uncovered at least 4 or 5 real bugs in code I care about. I see that these new options are not yet on master. Is there anything I can do to help get this patch accepted?
On Mon, Oct 24, 2016 at 04:26:49PM -0700, Jim Meyering wrote: > I have been using these new options (locally patched) to good effect. > While the vast majority of warning-triggering code has been > technically correct, using these has uncovered at least 4 or 5 real > bugs in code I care about. Awesome. Thanks for testing. > I see that these new options are not yet on master. Is there anything > I can do to help get this patch accepted? If you could get me 48 hour days that would help :) Sorry, I just ran out of time. I am just a spare time gcc hacker and somehow my spare time disappeared. I think the only thing "blocking" the patch from going in is that nobody made a decission on how the actual warning option should be named. I think the suggestion for -Wshadow=[global|local|compatible-local] is the right one. With -Wshadow being an alias for -Wshadow=global. But since there are already gcc versions out there that accept -Wshadow-local and -Wshadow-compatible-local (and you can find some configure scripts that already check for those options) it would be good to have those as (hidden) aliases. If people, some maintainer, agrees with that then we can do the .opt file hacking to make it so. Cheers, Mark
diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c index 8f49c35..31f83d8 100644 --- a/gcc/c/c-decl.c +++ b/gcc/c/c-decl.c @@ -2735,7 +2735,9 @@ warn_if_shadowing (tree new_decl) struct c_binding *b; /* Shadow warnings wanted? */ - if (!warn_shadow + if (!(warn_shadow + || warn_shadow_local + || warn_shadow_compatible_local) /* No shadow warnings for internally generated vars. */ || DECL_IS_BUILTIN (new_decl) /* No shadow warnings for vars made for inlining. */ @@ -2759,9 +2761,21 @@ warn_if_shadowing (tree new_decl) break; } else if (TREE_CODE (old_decl) == PARM_DECL) - warned = warning (OPT_Wshadow, - "declaration of %q+D shadows a parameter", - new_decl); + { + enum opt_code warning_code; + + /* If '-Wshadow-compatible-local' is specified without other + -Wshadow flags, we will warn only when the types of the + shadowing variable (i.e. new_decl) and the shadowed variable + (old_decl) are compatible. */ + if (comptypes (TREE_TYPE (old_decl), TREE_TYPE (new_decl))) + warning_code = OPT_Wshadow_compatible_local; + else + warning_code = OPT_Wshadow_local; + warned = warning (warning_code, + "declaration of %q+D shadows a parameter", + new_decl); + } else if (DECL_FILE_SCOPE_P (old_decl)) { /* Do not warn if a variable shadows a function, unless @@ -2784,8 +2798,21 @@ warn_if_shadowing (tree new_decl) break; } else - warned = warning (OPT_Wshadow, "declaration of %q+D shadows a " - "previous local", new_decl); + { + enum opt_code warning_code; + + /* If '-Wshadow-compatible-local' is specified without other + -Wshadow flags, we will warn only when the types of the + shadowing variable (i.e. new_decl) and the shadowed variable + (old_decl) are compatible. */ + if (comptypes (TREE_TYPE (old_decl), TREE_TYPE (new_decl))) + warning_code = OPT_Wshadow_compatible_local; + else + warning_code = OPT_Wshadow_local; + warned = warning (warning_code, + "declaration of %q+D shadows a previous local", + new_decl); + } if (warned) inform (DECL_SOURCE_LOCATION (old_decl), diff --git a/gcc/common.opt b/gcc/common.opt index fa1c036..17d54e7 100644 --- a/gcc/common.opt +++ b/gcc/common.opt @@ -661,7 +661,15 @@ Warn about returning a pointer/reference to a local or temporary variable. Wshadow Common Var(warn_shadow) Warning -Warn when one local variable shadows another. +Warn when one variable shadows another. + +Wshadow-local +Common Var(warn_shadow_local) Warning EnabledBy(Wshadow) +Warn when one local variable shadows another local variable or parameter. + +Wshadow-compatible-local +Common Var(warn_shadow_compatible_local) Warning EnabledBy(Wshadow-local) +Warn when one local variable shadows another local variable or parameter of compatible type. Wstack-protector Common Var(warn_stack_protect) Warning diff --git a/gcc/cp/name-lookup.c b/gcc/cp/name-lookup.c index 022ab6a..291a7ea 100644 --- a/gcc/cp/name-lookup.c +++ b/gcc/cp/name-lookup.c @@ -1195,19 +1195,39 @@ pushdecl_maybe_friend_1 (tree x, bool is_friend) nowarn = true; } - if (warn_shadow && !nowarn) + if ((warn_shadow + || warn_shadow_local + || warn_shadow_compatible_local) + && !nowarn) { bool warned; + enum opt_code warning_code; + /* If '-Wshadow-compatible-local' is specified without other + -Wshadow flags, we will warn only when the type of the + shadowing variable (i.e. x) can be converted to that of + the shadowed parameter (oldlocal). The reason why we only + check if x's type can be converted to oldlocal's type + (but not the other way around) is because when users + accidentally shadow a parameter, more than often they + would use the variable thinking (mistakenly) it's still + the parameter. It would be rare that users would use the + variable in the place that expects the parameter but + thinking it's a new decl. */ + if (can_convert (TREE_TYPE (oldlocal), TREE_TYPE (x), + tf_none)) + warning_code = OPT_Wshadow_compatible_local; + else + warning_code = OPT_Wshadow_local; if (TREE_CODE (oldlocal) == PARM_DECL) - warned = warning_at (input_location, OPT_Wshadow, + warned = warning_at (input_location, warning_code, "declaration of %q#D shadows a parameter", x); else if (is_capture_proxy (oldlocal)) - warned = warning_at (input_location, OPT_Wshadow, + warned = warning_at (input_location, warning_code, "declaration of %qD shadows a lambda capture", x); else - warned = warning_at (input_location, OPT_Wshadow, + warned = warning_at (input_location, warning_code, "declaration of %qD shadows a previous local", x); diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index b2eaea7..e23ca52 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -290,6 +290,7 @@ Objective-C and Objective-C++ Dialects}. -Wpointer-arith -Wno-pointer-to-int-cast @gol -Wno-pragmas -Wredundant-decls -Wno-return-local-addr @gol -Wreturn-type -Wsequence-point -Wshadow -Wno-shadow-ivar @gol +-Wshadow-compatible-local -Wshadow-local @gol -Wshift-overflow -Wshift-overflow=@var{n} @gol -Wshift-count-negative -Wshift-count-overflow -Wshift-negative-value @gol -Wsign-compare -Wsign-conversion -Wfloat-conversion @gol @@ -5001,6 +5002,46 @@ explicit typedef, but not if it shadows a struct/class/enum. Do not warn whenever a local variable shadows an instance variable in an Objective-C method. +@item -Wshadow-local +@opindex Wshadow-local +@opindex Wno-shadow-local +Warn when a local variable shadows another local variable or parameter. +This warning is enabled by @option{-Wshadow}. + +@item -Wshadow-compatible-local +@opindex Wshadow-compatible-local +@opindex Wno-shadow-compatible-local +Warn when a local variable shadows another local variable or parameter +whose type is compatible with that of the shadowing variable. In C++, +type compatibility here means the type of the shadowing variable can be +converted to that of the shadowed variable. The creation of this flag +(in addition to @option{-Wshadow-local}) is based on the idea that when +a local variable shadows another one of incompatible type, it is most +likely intentional, not a bug or typo, as shown in the following example: + +@smallexample +@group +for (SomeIterator i = SomeObj.begin(); i != SomeObj.end(); ++i) +@{ + for (int i = 0; i < N; ++i) + @{ + ... + @} + ... +@} +@end group +@end smallexample + +Since the two variable @code{i} in the example above have incompatible types, +enabling only @option{-Wshadow-compatible-local} will not emit a warning. +Because their types are incompatible, if a programmer accidentally uses one +in place of the other, type checking will catch that and emit an error or +warning. So not warning (about shadowing) in this case will not lead to +undetected bugs. Use of this flag instead of @option{-Wshadow-local} can +possibly reduce the number of warnings triggered by intentional shadowing. + +This warning is enabled by @option{-Wshadow-local}. + @item -Wlarger-than=@var{len} @opindex Wlarger-than=@var{len} @opindex Wlarger-than-@var{len} diff --git a/gcc/testsuite/g++.dg/warn/Wshadow-compatible-local-1.C b/gcc/testsuite/g++.dg/warn/Wshadow-compatible-local-1.C new file mode 100644 index 0000000..b0422fa --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wshadow-compatible-local-1.C @@ -0,0 +1,63 @@ +/* { dg-do compile } */ +/* { dg-options -Wshadow-compatible-local } */ + +class Bar { +}; + +class ChildBar : public Bar { +}; + +Bar bar; + +class Foo { + private: + int val; + + public: + int func1(int x) { + int val; + val = x; + return val; + } + + int func2(int i) { // { dg-message "note: shadowed declaration is here" } + int a = 3; // { dg-message "note: shadowed declaration is here" } + + for (int i = 0; i < 5; ++i) { // { dg-warning "shadows a parameter" } + for (int i = 0; i < 3; ++i) { // { dg-warning "shadows a previous local" } + int a = i; // { dg-warning "shadows a previous local" } + func1(a); + } + } + + return a; + } + + int func3() { + int bar; + float func1 = 0.3; + int f = 5; // { dg-message "note: shadowed declaration is here" } + + if (func1 > 1) { + float f = 2.0; // { dg-warning "shadows a previous local" } + bar = f; + } + else + bar = 1; + return bar; + } + + void func4() { + Bar *bar; // { dg-bogus "shadowed declaration" } + ChildBar *cbp; // { dg-bogus "shadowed declaration" } + Bar *bp; // { dg-message "note: shadowed declaration is here" } + if (val) { + int bar; // { dg-bogus "shadows a previous local" } + Bar *cbp; // { dg-bogus "shadows a previous local" } + ChildBar *bp; // { dg-warning "shadows a previous local" } + func1(bar); + } + } +}; + +// { dg-message "note: shadowed declaration" "" { target *-*-* } 26 } diff --git a/gcc/testsuite/g++.dg/warn/Wshadow-local-1.C b/gcc/testsuite/g++.dg/warn/Wshadow-local-1.C new file mode 100644 index 0000000..9737e23 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wshadow-local-1.C @@ -0,0 +1,35 @@ +/* { dg-do compile } */ +/* { dg-options -Wshadow-local } */ + +struct status +{ + int member; + void foo2 (); + + inline static int foo3 (int member) + { + return member; + } +}; + +int decl1; // { dg-bogus "shadowed declaration" } +int decl2; // { dg-bogus "shadowed declaration" } +void foo (struct status &status, + double decl1) // { dg-bogus "shadows a global" } +{ +} + +void foo1 (int d) +{ + double d; // { dg-error "shadows a parameter" } +} + +void status::foo2 () +{ + int member; // { dg-bogus "shadows a member" } + int decl2; // { dg-bogus "shadows a global" } + int local; // { dg-message "note: shadowed declaration is here" } + { + int local; // { dg-warning "shadows a previous local" } + } +} diff --git a/gcc/testsuite/g++.dg/warn/Wshadow-local-2.C b/gcc/testsuite/g++.dg/warn/Wshadow-local-2.C new file mode 100644 index 0000000..c0d6741 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wshadow-local-2.C @@ -0,0 +1,63 @@ +/* { dg-do compile } */ +/* { dg-options -Wshadow-local } */ + +class Bar { +}; + +class ChildBar : public Bar { +}; + +Bar bar; // { dg-bogus "shadowed declaration" } + +class Foo { + private: + int val; + + public: + int func1(int x) { + int val; // { dg-bogus "shadows a member" } + val = x; + return val; + } + + int func2(int i) { // { dg-message "shadowed declaration is here" } + int a = 3; // { dg-message "shadowed declaration is here" } + + for (int i = 0; i < 5; ++i) { // { dg-warning "shadows a parameter" } + for (int i = 0; i < 3; ++i) { // { dg-warning "shadows a previous local" } + int a = i; // { dg-warning "shadows a previous local" } + func1(a); + } + } + + return a; + } + + int func3() { + int bar; // { dg-bogus "shadows a global" } + float func1 = 0.3; // { dg-bogus "shadows a member" } + int f = 5; // { dg-message "shadowed declaration is here" } + + if (func1 > 1) { + float f = 2.0; // { dg-warning "shadows a previous local" } + bar = f; + } + else + bar = 1; + return bar; + } + + void func4() { + Bar *bar; // { dg-message "shadowed declaration is here" } + ChildBar *cbp; // { dg-message "shadowed declaration is here" } + Bar *bp; // { dg-message "shadowed declaration is here" } + if (val) { + int bar; // { dg-warning "shadows a previous local" } + Bar *cbp; // { dg-warning "shadows a previous local" } + ChildBar *bp; // { dg-warning "shadows a previous local" } + func1(bar); + } + } +}; + +// { dg-message "shadowed declaration is here" "" { target *-*-* } 26 } diff --git a/gcc/testsuite/gcc.dg/Wshadow-compatible-local-1.c b/gcc/testsuite/gcc.dg/Wshadow-compatible-local-1.c new file mode 100644 index 0000000..80f43de --- /dev/null +++ b/gcc/testsuite/gcc.dg/Wshadow-compatible-local-1.c @@ -0,0 +1,36 @@ +/* { dg-do compile } */ +/* { dg-options "-Wshadow-compatible-local" } */ + +struct Bar { +}; + +struct Bar bar; /* { dg-bogus "shadowed declaration" } */ + +int val; /* { dg-bogus "shadowed declaration" } */ + +int func1(int x) { /* { dg-bogus "shadowed declaration" } */ + int val; /* { dg-bogus "shadows a global" } */ + val = x; + return val; +} + +int func2(int i) { + int a = 3; /* { dg-message "shadowed declaration" } */ + int j; /* { dg-message "shadowed declaration" } */ + + for (j = 0; j < i; ++j) { + int a = j; /* { dg-warning "shadows a previous local" } */ + int j = a + 1; /* { dg-warning "shadows a previous local" } */ + func1(j); + } + + return a; +} + +void func4() { + struct Bar bar; /* { dg-bogus "shadowed declaration" } */ + if (val) { + int bar; /* { dg-bogus "shadows a previous local" } */ + func1(bar); + } +} diff --git a/gcc/testsuite/gcc.dg/Wshadow-local-1.c b/gcc/testsuite/gcc.dg/Wshadow-local-1.c new file mode 100644 index 0000000..df70c11 --- /dev/null +++ b/gcc/testsuite/gcc.dg/Wshadow-local-1.c @@ -0,0 +1,22 @@ +/* { dg-do compile } */ +/* { dg-options "-Wshadow-local" } */ + +int decl1; /* should not warn */ +void foo (double decl1) /* should not warn */ +{ +} + +void foo2 (int d) /* { dg-message "shadowed declaration" } */ +{ + { + double d; /* { dg-warning "shadows a parameter" } */ + } +} + +void foo3 () +{ + int local; /* { dg-message "shadowed declaration" } */ + { + int local; /* { dg-warning "shadows a previous local" } */ + } +} diff --git a/gcc/testsuite/gcc.dg/Wshadow-local-2.c b/gcc/testsuite/gcc.dg/Wshadow-local-2.c new file mode 100644 index 0000000..90d4e0f --- /dev/null +++ b/gcc/testsuite/gcc.dg/Wshadow-local-2.c @@ -0,0 +1,49 @@ +/* { dg-do compile } */ +/* { dg-options "-Wshadow-local" } */ + +struct Bar { +}; + +struct Bar bar; /* { dg-bogus "shadowed declaration" } */ + +int val; /* { dg-bogus "shadowed declaration" } */ + +int func1(int x) { /* { dg-bogus "shadowed declaration" } */ + int val; /* { dg-bogus "shadows a global" } */ + val = x; + return val; +} + +int func2(int i) { + int a = 3; /* { dg-message "shadowed declaration" } */ + int j; /* { dg-message "shadowed declaration" } */ + + for (j = 0; j < i; ++j) { + int a = j; /* { dg-warning "shadows a previous local" } */ + int j = a + 1; /* { dg-warning "shadows a previous local" } */ + func1(j); + } + + return a; +} + +int func3() { + int bar; /* { dg-bogus "shadows a global" } */ + float func1 = 0.3; /* { dg-bogus "shadows a global" } */ + + if (func1 > 1) + bar = 2; + else + bar = 1; + return bar; +} + +void func4() { + struct Bar bar; /* { dg-message "shadowed declaration" } */ + if (val) { + int bar; /* { dg-warning "shadows a previous local" } */ + func1(bar); + } +} + +/* { dg-bogus "shadows a global" "" { target *-*-* } 42 } */ diff --git a/gcc/testsuite/gcc.dg/Wshadow-local-3.c b/gcc/testsuite/gcc.dg/Wshadow-local-3.c new file mode 100644 index 0000000..429df37 --- /dev/null +++ b/gcc/testsuite/gcc.dg/Wshadow-local-3.c @@ -0,0 +1,9 @@ +/* { dg-do compile } */ +/* { dg-options "-Wno-shadow" } */ + +void func() { + int i; + { + int i; /* should not warn */ + } +}